#252: Handling unsupported column in SELECT * (#255)

* #252: Handling unsupported column in `SELECT *`

* #252: Narrowed down access on extracted methods. Removed superfluous log output.

* #252: Removed superfluous log message.

jdbc-adapter/launch/virtual-schema debug listen on port 8000.launch

@@ -6,10 +6,13 @@
 <listAttribute key="org.eclipse.debug.core.MAPPED_RESOURCE_TYPES">
 <listEntry value="4"/>
 </listAttribute>
-<booleanAttribute key="org.eclipse.jdt.launching.ALLOW_TERMINATE" value="true"/>
+<stringAttribute key="org.eclipse.debug.core.source_locator_id" value="org.eclipse.jdt.launching.sourceLocator.JavaAdvancedSourceLookupDirector"/>
+<stringAttribute key="org.eclipse.debug.core.source_locator_memento" value="&lt;?xml version=&quot;1.0&quot; encoding=&quot;UTF-8&quot; standalone=&quot;no&quot;?&gt;&#10;&lt;sourceLookupDirector&gt;&#10;&lt;sourceContainers duplicates=&quot;false&quot;&gt;&#10;&lt;container memento=&quot;&amp;lt;?xml version=&amp;quot;1.0&amp;quot; encoding=&amp;quot;UTF-8&amp;quot; standalone=&amp;quot;no&amp;quot;?&amp;gt;&amp;#10;&amp;lt;folder nest=&amp;quot;false&amp;quot; path=&amp;quot;/virtual-schema-common-java/src/main/java&amp;quot;/&amp;gt;&amp;#10;&quot; typeId=&quot;org.eclipse.debug.core.containerType.folder&quot;/&gt;&#10;&lt;container memento=&quot;&amp;lt;?xml version=&amp;quot;1.0&amp;quot; encoding=&amp;quot;UTF-8&amp;quot; standalone=&amp;quot;no&amp;quot;?&amp;gt;&amp;#10;&amp;lt;default/&amp;gt;&amp;#10;&quot; typeId=&quot;org.eclipse.debug.core.containerType.default&quot;/&gt;&#10;&lt;/sourceContainers&gt;&#10;&lt;/sourceLookupDirector&gt;&#10;"/>
+<booleanAttribute key="org.eclipse.jdt.launching.ALLOW_TERMINATE" value="false"/>
 <mapAttribute key="org.eclipse.jdt.launching.CONNECT_MAP">
 <mapEntry key="connectionLimit" value="1"/>
 <mapEntry key="port" value="8000"/>
+<mapEntry key="timeout" value="20000"/>
 </mapAttribute>
 <stringAttribute key="org.eclipse.jdt.launching.PROJECT_ATTR" value="virtualschema-jdbc-adapter"/>
 <stringAttribute key="org.eclipse.jdt.launching.VM_CONNECTOR_ID" value="org.eclipse.jdt.launching.socketListenConnector"/>

jdbc-adapter/pom.xml

@@ -17,7 +17,7 @@
         <junit.version>5.4.2</junit.version>
         <junit.platform.version>1.4.2</junit.platform.version>
         <maven.surefire.version>2.22.1</maven.surefire.version>
-        <vscommon.version>7.2.1</vscommon.version>
+        <vscommon.version>7.3.0</vscommon.version>
     </properties>
     <distributionManagement>
         <repository>
@@ -53,8 +53,8 @@
         </dependency>
         <dependency>
             <groupId>org.hamcrest</groupId>
-            <artifactId>hamcrest-junit</artifactId>
-            <version>2.0.0.0</version>
+            <artifactId>hamcrest</artifactId>
+            <version>2.1</version>
             <scope>test</scope>
         </dependency>
         <dependency>

jdbc-adapter/virtualschema-jdbc-adapter/src/main/java/com/exasol/adapter/dialects/SqlGenerationVisitor.java

@@ -130,7 +130,7 @@ protected String representAnyColumnInSelectList() {
      * <p>
      * Override this method in case conversions are necessary.
      * </p>
-     * 
+     *
      * @param selectList list of columns (or expressions) in the <code>SELECT</code> part
      * @return always <code>"true"</code>
      */
@@ -198,7 +198,7 @@ public String visit(final SqlJoin join) throws AdapterException {
     @Override
     public String visit(final SqlGroupBy groupBy) throws AdapterException {
         if ((groupBy.getExpressions() == null) || groupBy.getExpressions().isEmpty()) {
-            throw new RuntimeException("Unexpected internal state (empty group by)");
+            throw new IllegalStateException("Unexpected internal state (empty group by)");
         }
         final List<String> selectElement = new ArrayList<>();
         for (final SqlNode node : groupBy.getExpressions()) {
@@ -229,13 +229,13 @@ public String visit(final SqlFunctionAggregate function) throws AdapterException
 
     @Override
     public String visit(final SqlFunctionAggregateGroupConcat function) throws AdapterException {
+        validateSingleAgrumentFunctionParameter(function);
         final StringBuilder builder = new StringBuilder();
         builder.append(function.getFunctionName());
         builder.append("(");
         if (function.hasDistinct()) {
             builder.append("DISTINCT ");
         }
-        assert ((function.getArguments().size() == 1) && (function.getArguments().get(0) != null));
         builder.append(function.getArguments().get(0).accept(this));
         if (function.hasOrderBy()) {
             builder.append(" ");
@@ -252,6 +252,13 @@ public String visit(final SqlFunctionAggregateGroupConcat function) throws Adapt
         return builder.toString();
     }
 
+    private void validateSingleAgrumentFunctionParameter(final SqlFunctionAggregateGroupConcat function) {
+        if ((function.getArguments().size() != 1) || (function.getArguments().get(0) == null)) {
+            throw new IllegalArgumentException(
+                    "Function AGGREGATE GROUP CONCAT must have exactly one non-NULL parameter.");
+        }
+    }
+
     @Override
     public String visit(final SqlFunctionScalar function) throws AdapterException {
         final List<String> argumentsSql = new ArrayList<>();
@@ -312,25 +319,36 @@ public String visit(final SqlFunctionScalarCase function) throws AdapterExceptio
 
     @Override
     public String visit(final SqlFunctionScalarCast function) throws AdapterException {
-
+        validateSingleAgrumentFunctionParameter(function);
         final StringBuilder builder = new StringBuilder();
         builder.append("CAST");
         builder.append("(");
-        assert ((function.getArguments().size() == 1) && (function.getArguments().get(0) != null));
         builder.append(function.getArguments().get(0).accept(this));
         builder.append(" AS ");
         builder.append(function.getDataType());
         builder.append(")");
         return builder.toString();
     }
 
+    private void validateSingleAgrumentFunctionParameter(final SqlFunctionScalarCast function) {
+        if ((function.getArguments().size() != 1) || (function.getArguments().get(0) == null)) {
+            throw new IllegalArgumentException("Function CAST must have exactly one non-NULL parameter.");
+        }
+    }
+
     @Override
     public String visit(final SqlFunctionScalarExtract function) throws AdapterException {
-        assert ((function.getArguments().size() == 1) && (function.getArguments().get(0) != null));
+        validateSingleAgrumentFunctionParameter(function);
         final String expression = function.getArguments().get(0).accept(this);
         return function.getFunctionName() + "(" + function.getToExtract() + " FROM " + expression + ")";
     }
 
+    private void validateSingleAgrumentFunctionParameter(final SqlFunctionScalarExtract function) {
+        if ((function.getArguments().size() != 1) || (function.getArguments().get(0) == null)) {
+            throw new IllegalArgumentException("Function EXTRACT must have exactly one non-NULL parameter.");
+        }
+    }
+
     @Override
     public String visit(final SqlLimit limit) {
         String offsetSql = "";

jdbc-adapter/virtualschema-jdbc-adapter/src/main/java/com/exasol/adapter/dialects/oracle/OracleSqlGenerationVisitor.java

@@ -4,6 +4,7 @@
 import static com.exasol.adapter.sql.ScalarFunction.*;
 
 import java.util.*;
+import java.util.logging.Logger;
 
 import com.exasol.adapter.AdapterException;
 import com.exasol.adapter.dialects.*;
@@ -15,7 +16,7 @@
  * This class generates SQL queries for the {@link OracleSqlGenerationVisitor}.
  */
 public class OracleSqlGenerationVisitor extends SqlGenerationVisitor {
-    // If set to true, the SELECT list elements will get aliases such as c1, c2, ...
+    public static final Logger LOGGER = Logger.getLogger(OracleSqlGenerationVisitor.class.getName());
     private boolean requiresSelectListAliasesForLimit = false;
     private static final String TIMESTAMP_FORMAT = "'YYYY-MM-DD HH24:MI:SS.FF3'";
     private static final List<String> TYPE_NAMES_REQUIRING_CAST = List.of("TIMESTAMP", "INTERVAL", "BINARY_FLOAT",
@@ -62,7 +63,7 @@ Set<ScalarFunction> getScalarFunctionsCast() {
      * SELECT c1, c2, ... FROM ( SELECT LIMIT_SUBSELECT.*, ROWNUM ROWNUM_SUB FROM ( &lt;query-with-aliases&gt; )
      * LIMIT_SUBSELECT WHERE ROWNUM &lt;= 30 ) WHERE ROWNUM_SUB &gt; 20
      *
-     * The rownum filter is evaluated before ORDER BY, which is why we need sub-selects
+     * The ROWNUM filter is evaluated before ORDER BY, which is why we need sub-selects
      */
     @Override
     public String visit(final SqlStatementSelect select) throws AdapterException {
@@ -145,27 +146,28 @@ public String visit(final SqlSelectList selectList) throws AdapterException {
     private String getSqlSelectList(final SqlSelectList selectList) throws AdapterException {
         final List<String> selectListElements = new ArrayList<>();
         if (selectList.isSelectStar()) {
-            // Do as if the user has all columns in select list
-            final SqlStatementSelect select = (SqlStatementSelect) selectList.getParent();
-            final boolean selectListRequiresCasts = isSelectListRequiresCasts(selectList, selectListElements, select);
-            if (!this.requiresSelectListAliasesForLimit && !selectListRequiresCasts) {
-                selectListElements.clear();
-                selectListElements.add("*");
-            }
+            getSelectStarList(selectList, selectListElements);
         } else {
             for (final SqlNode node : selectList.getExpressions()) {
                 selectListElements.add(node.accept(this));
             }
         }
         if (this.requiresSelectListAliasesForLimit) {
-            // Add aliases to select list elements
-            for (int i = 0; i < selectListElements.size(); i++) {
-                selectListElements.set(i, selectListElements.get(i) + " AS c" + i);
-            }
+            addColumnAliases(selectListElements);
         }
         return String.join(", ", selectListElements);
     }
 
+    private void getSelectStarList(final SqlSelectList selectList, final List<String> selectListElements)
+            throws AdapterException {
+        final SqlStatementSelect select = (SqlStatementSelect) selectList.getParent();
+        final boolean selectListRequiresCasts = isSelectListRequiresCasts(selectList, selectListElements, select);
+        if (!this.requiresSelectListAliasesForLimit && !selectListRequiresCasts) {
+            selectListElements.clear();
+            selectListElements.add("*");
+        }
+    }
+
     private boolean isSelectListRequiresCasts(final SqlSelectList selectList, final List<String> selectListElements,
             final SqlStatementSelect select) throws AdapterException {
         boolean selectListRequiresCasts = false;
@@ -228,11 +230,17 @@ private boolean checkIfNeedToCastNumberToDecimal(final SqlColumn column) {
                 && (columnType.getScale() == castNumberToDecimalType.getScale());
     }
 
+    private void addColumnAliases(final List<String> selectListElements) {
+        for (int i = 0; i < selectListElements.size(); i++) {
+            selectListElements.set(i, selectListElements.get(i) + " AS c" + i);
+        }
+    }
+
+    // Limit is realized via a {@code ROWNUM} filter in Oracle (< 12c) Oracle 12c introduced nice syntax for limit and
+    // offset
+    // functionality: "OFFSET 4 ROWS FETCH NEXT 4 ROWS ONLY"
     @Override
     public String visit(final SqlLimit limit) {
-        // Limit is realized via a rownum filter in Oracle (< 12c)
-        // Oracle 12c introduced nice syntax for limit and offset functionality: "OFFSET 4 ROWS FETCH NEXT 4 ROWS ONLY".
-        // Nice to have to add this.
         return "";
     }
 
@@ -262,7 +270,7 @@ private String getProjectionString(final SqlColumn column, final String projecti
         final String typeName = ColumnAdapterNotes
                 .deserialize(column.getMetadata().getAdapterNotes(), column.getMetadata().getName()).getTypeName();
         if (typeName.startsWith("INTERVAL") || typeName.equals("BINARY_FLOAT") || typeName.equals("BINARY_DOUBLE")) {
-            return "TO_CHAR(" + projectionString + ")";
+            return createToChar(projectionString);
         } else if (typeName.startsWith("TIMESTAMP")
                 && (((OracleSqlDialect) dialect).getImportType() == ImportType.JDBC)) {
             return "TO_TIMESTAMP(TO_CHAR(" + projectionString + ", " + TIMESTAMP_FORMAT + "), " + TIMESTAMP_FORMAT
@@ -278,21 +286,29 @@ private String getProjectionString(final SqlColumn column, final String projecti
         }
     }
 
+    public String createToChar(final String operand) {
+        return "TO_CHAR(" + operand + ")";
+    }
+
     private String getNumberProjectionString(final SqlColumn column, final String projectionString,
             final OracleSqlDialect dialect) {
         if (column.getMetadata().getType().getExaDataType() == DataType.ExaDataType.VARCHAR) {
-            return "TO_CHAR(" + projectionString + ")";
+            return createToChar(projectionString);
         } else {
             if (checkIfNeedToCastNumberToDecimal(column)) {
                 final DataType castNumberToDecimalType = dialect.getOracleNumberTargetType();
-                return "CAST(" + projectionString + " AS DECIMAL(" + castNumberToDecimalType.getPrecision() + ","
-                        + castNumberToDecimalType.getScale() + "))";
+                return cast(projectionString, "DECIMAL(" + castNumberToDecimalType.getPrecision() + ","
+                        + castNumberToDecimalType.getScale() + ")");
             } else {
                 return projectionString;
             }
         }
     }
 
+    private String cast(final String value, final String as) {
+        return "CAST(" + value + " AS " + as + ")";
+    }
+
     @Override
     public String visit(final SqlLiteralExactnumeric literal) {
         final String literalString = literal.getValue().toString();
@@ -302,7 +318,7 @@ public String visit(final SqlLiteralExactnumeric literal) {
     private String getLiteralString(final String literalString, final boolean b, final SqlNode parent) {
         final boolean isDirectlyInSelectList = (b && (parent.getType() == SqlNodeType.SELECT_LIST));
         if (isDirectlyInSelectList) {
-            return "TO_CHAR(" + literalString + ")";
+            return createToChar(literalString);
         }
         return literalString;
     }
@@ -363,14 +379,13 @@ private String getOrderByString(final SqlFunctionAggregateGroupConcat function)
 
     @Override
     public String visit(final SqlFunctionAggregate function) throws AdapterException {
-        String sql = super.visit(function);
         final boolean isDirectlyInSelectList = (function.hasParent()
                 && (function.getParent().getType() == SqlNodeType.SELECT_LIST));
         if (isDirectlyInSelectList && this.aggregateFunctionsCast.contains(function.getFunction())) {
             // Cast to FLOAT because result set metadata has precision = 0, scale = 0
-            sql = "CAST(" + sql + " AS FLOAT)";
+            return cast(super.visit(function), "FLOAT");
         }
-        return sql;
+        return super.visit(function);
     }
 
     @Override
@@ -449,7 +464,7 @@ public String visit(final SqlFunctionScalar function) throws AdapterException {
                 && (function.getParent().getType() == SqlNodeType.SELECT_LIST));
         if (isDirectlyInSelectList && this.scalarFunctionsCast.contains(function.getFunction())) {
             // Cast to FLOAT because result set metadata has precision = 0, scale = 0
-            sql = "CAST(" + sql + " AS FLOAT)";
+            sql = cast(sql, "FLOAT");
         }
         return sql;
     }

jdbc-adapter/virtualschema-jdbc-adapter/src/main/java/com/exasol/adapter/jdbc/ResultSetMetadataReader.java

@@ -4,15 +4,16 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 import com.exasol.adapter.dialects.JdbcTypeDescription;
 import com.exasol.adapter.metadata.DataType;
 
 /**
  * This class creates a textual description of the result columns of a push-down query.
  * <p>
- * The columns description is necessary to prepare the <code>IMPORT</code> statement in which the push-down query
- * is executed.
+ * The columns description is necessary to prepare the <code>IMPORT</code> statement in which the push-down query is
+ * executed.
  */
 public class ResultSetMetadataReader {
     private static final Logger LOGGER = Logger.getLogger(ResultSetMetadataReader.class.getName());
@@ -42,6 +43,7 @@ public String describeColumns(final String query) {
         try (final PreparedStatement statement = this.connection.prepareStatement(query)) {
             final ResultSetMetaData metadata = statement.getMetaData();
             final List<DataType> types = mapResultMetadataToExasolDataTypes(metadata);
+            validateColumnTypes(types, query);
             final String columnsDescription = createColumnDescriptionFromDataTypes(types);
             LOGGER.fine(() -> "Columns description: " + columnsDescription);
             return columnsDescription;
@@ -53,6 +55,22 @@ public String describeColumns(final String query) {
         }
     }
 
+    private void validateColumnTypes(final List<DataType> types, final String query) {
+        final List<Integer> illegalColumns = new ArrayList<>();
+        int column = 1;
+        for (final DataType type : types) {
+            if (!type.isSupported()) {
+                illegalColumns.add(column);
+            }
+            ++column;
+        }
+        if (!illegalColumns.isEmpty()) {
+            throw new RemoteMetadataReaderException("Unsupported data type(s) in column(s) "
+                    + illegalColumns.stream().map(String::valueOf).collect(Collectors.joining(", "))
+                    + " in query. Please remove those colums from your query:\n" + query);
+        }
+    }
+
     private String createColumnDescriptionFromDataTypes(final List<DataType> types) {
         final StringBuilder builder = new StringBuilder();
         int columnNumber = 1;

jdbc-adapter/virtualschema-jdbc-adapter/src/test/java/com/exasol/adapter/dialects/AbstractSqlDialectFactoryTest.java

@@ -1,7 +1,7 @@
 package com.exasol.adapter.dialects;
 
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
-import static org.hamcrest.junit.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertAll;
 
 import java.sql.Connection;