#149: Workaround for GROUP BY <integer>

doc/changes/changes_11.0.1.md

@@ -1,9 +1,23 @@
-# Virtual Schema Common JDBC 11.0.1, released 2023-??-??
+# Virtual Schema Common JDBC 11.0.1, released 2023-07-11
 
-Code name:
+Code name: Fix Issue With Integer Constants in `GROUP BY`
 
 ## Summary
 
+This release fixes an issue with queries using `DISTINCT` with integer constants. The Exasol SQL processor turns `DISTINCT <integer>` into `GROUP BY <integer>` before push-down as an optimization. The adapter must not feed this back as Exasol interprets integers in `GROUP BY` clauses as column numbers which could lead to invalid results or the following error:
+
+```
+42000:Wrong column number. Too small value 0 as select list column reference in GROUP BY (smallest possible value is 1)
+```
+
+To fix this, VSCJDBC now replaces integer constants in `GROUP BY` clauses with a constant string.
+
+Please that you can still safely use `GROUP BY <column-number>` in your original query, since Exasol internally converts this to `GROUP BY "<column-name>"`, so that the virtual schema adapter can tell both situations apart.
+
+## Bugfixes
+
+* #149: Fixed issue with integer constants in `GROUP BY`
+
 ## Documentation
 
 * #147: Removed duplicate documentation about logging

src/main/java/com/exasol/adapter/dialects/rewriting/SqlGenerationVisitor.java

@@ -3,6 +3,7 @@
 import java.text.DecimalFormat;
 import java.text.NumberFormat;
 import java.util.*;
+import java.util.logging.Logger;
 
 import com.exasol.adapter.AdapterException;
 import com.exasol.adapter.adapternotes.ColumnAdapterNotesJsonConverter;
@@ -25,13 +26,14 @@
  * {@link SqlDialect#getSqlGenerator(SqlGenerationContext)}.
  * </p>
  *
- * Note on operator associativity and parenthesis generation: Currently we use parenthesis almost always. Without
- * parenthesis, two SqlNode graphs with different semantic lead to "select 1 = 1 - 1 + 1". Also "SELECT NOT NOT TRUE"
- * needs to be written as "SELECT NOT (NOT TRUE)" to work at all, whereas SELECT NOT TRUE works fine without
- * parentheses. Currently we make inflationary use of parenthesis to to enforce the right semantic, but hopefully there
- * is a better way.
+ * Note on operator associativity and parenthesis generation: Currently we almost always use parenthesis. Without
+ * parenthesis, two {@link SqlNode} graphs with different semantic lead to {@code select 1 = 1 - 1 + 1}. Also
+ * {@code SELECT NOT NOT TRUE} needs to be written as {@code SELECT NOT (NOT TRUE)} to work at all, whereas
+ * {@code SELECT NOT TRUE} works fine without parentheses. Currently we make inflationary use of parenthesis to enforce
+ * the right semantic, but hopefully there is a better way.
  */
 public class SqlGenerationVisitor implements SqlNodeVisitor<String>, SqlGenerator {
+    private static final Logger LOGGER = Logger.getLogger(SqlGenerationVisitor.class.getName());
     private final SqlDialect dialect;
     private final SqlGenerationContext context;
 
@@ -240,11 +242,33 @@ public String visit(final SqlFunctionAggregateListagg sqlFunctionAggregateListag
     public String visit(final SqlGroupBy groupBy) throws AdapterException {
         final List<String> selectElement = new ArrayList<>();
         for (final SqlNode node : groupBy.getExpressions()) {
-            selectElement.add(node.accept(this));
+            final SqlNode replacement = workaroundGroupByInteger(node);
+            selectElement.add(replacement.accept(this));
         }
         return String.join(", ", selectElement);
     }
 
+    /**
+     * Replace an unsupported expression in a {@code GROUP BY} clause with a supported one or return it unchanged.
+     * 
+     * @implNote This replaces numeric literals with the corresponding string value, as Exasol interprets
+     *           {@code GROUP BY <integer-constant>} as column number &mdash; which is not what the user intended. Also,
+     *           please note that `GROUP BY <constant> always leads to grouping with a single group, regardless of the
+     *           actual value of the constant (except for {@code FALSE}, which is reserved).
+     * 
+     * @param node the original {@code GROUP BY} expression
+     * @return an new, alternative expression or the original expression if no replacement is necessary
+     */
+    private SqlNode workaroundGroupByInteger(final SqlNode node) {
+        if (node instanceof SqlLiteralExactnumeric) {
+            final SqlLiteralExactnumeric numericNode = (SqlLiteralExactnumeric) node;
+            LOGGER.fine(() -> "Replacing numeric literal " + numericNode.getValue() + " with a string in GROUP BY");
+            return new SqlLiteralString(String.valueOf(numericNode.getValue()));
+        } else {
+            return node;
+        }
+    }
+
     @Override
     public String visit(final SqlFunctionAggregate function) throws AdapterException {
         final StringBuilder builder = new StringBuilder();

src/test/java/com/exasol/adapter/dialects/SqlGenerationVisitorTest.java

@@ -420,13 +420,26 @@ void testVisitSqlTableCatalogAndSchemaQualifiedQuoting() {
 
     @Test
     void testVisitGroupByQuoting() throws AdapterException {
-        final List<SqlNode> expressions = new ArrayList<>();
-        expressions.add(new SqlColumn(1, ColumnMetadata.builder().name("\" a '").type(DataType.createBool()).build()));
-        expressions.add(new SqlColumn(2, ColumnMetadata.builder().name("\" b '").type(DataType.createBool()).build()));
-        final SqlGroupBy groupBy = new SqlGroupBy(expressions);
+        final SqlGroupBy groupBy = new SqlGroupBy(List.of( //
+                new SqlColumn(1, ColumnMetadata.builder().name("\" a '").type(DataType.createBool()).build()), //
+                new SqlColumn(2, ColumnMetadata.builder().name("\" b '").type(DataType.createBool()).build())));
         assertThat(sqlGenerationVisitor.visit(groupBy), equalTo("\"\"\" a '\", \"\"\" b '\""));
     }
 
+    @Test
+    void testVisitGroupByReplacesIntegerLiterals() throws AdapterException {
+        final SqlGroupBy groupBy = new SqlGroupBy(List.of( //
+                new SqlColumn(1, ColumnMetadata.builder().name("a").type(DataType.createBool()).build()), //
+                new SqlLiteralExactnumeric(BigDecimal.valueOf(42))));
+        assertThat(sqlGenerationVisitor.visit(groupBy), equalTo("\"a\", '42'"));
+    }
+
+    @Test
+    void testVisitGroupByDoesNotModifyStringLiterals() throws AdapterException {
+        final SqlGroupBy groupBy = new SqlGroupBy(List.of(new SqlLiteralString("literal")));
+        assertThat(sqlGenerationVisitor.visit(groupBy), equalTo("'literal'"));
+    }
+
     @Test
     void testVisitSqlFunctionAggregateQuoting() throws AdapterException {
         final List<SqlNode> arguments = new ArrayList<>();