diff --git a/doc/changes/changes_18.0.2.md b/doc/changes/changes_18.0.2.md index 26b5dd6..3ae4b4e 100644 --- a/doc/changes/changes_18.0.2.md +++ b/doc/changes/changes_18.0.2.md @@ -24,6 +24,7 @@ Code name: Improve code quality * #317: Applied small allocation cleanups in request parsing, logging timestamp formatting, and schema metadata JSON conversion. * #308: Fixed `SqlLimit` so `limit` and `offset` stay non-negative for the full object lifetime by making the node immutable and aligning the validation message with the accepted zero values. * #307: Fixed `PushdownSqlRenderer` so `HASHTYPE` data types include `bytesize` in rendered pushdown SQL JSON. +* #315: Improved `PushdownSqlParser` lookup and traversal performance by caching involved table metadata, defensively copying the metadata list, and replacing `Stack`-based traversals with `ArrayDeque`. * #313: Fixed mutable DTO and SQL AST collection handling by adding defensive copies, unmodifiable getters, null-to-empty normalization, and constructor-time `SqlOrderBy` validation. * #312: Fixed logging utility resource and handler lifecycle issues by closing version metadata streams, falling back when the thread context class loader is missing, defaulting absent version properties to `UNKNOWN`, closing replaced root log handlers, and resetting closed remote socket handlers before reuse. * #316: Fixed `LoggingConfiguration` so invalid `LOG_LEVEL` values now fail with a structured error message that names the invalid value and lists the available log levels. diff --git a/src/main/java/com/exasol/adapter/request/parser/PushdownSqlParser.java b/src/main/java/com/exasol/adapter/request/parser/PushdownSqlParser.java index 8fb2f6b..e10bc12 100644 --- a/src/main/java/com/exasol/adapter/request/parser/PushdownSqlParser.java +++ b/src/main/java/com/exasol/adapter/request/parser/PushdownSqlParser.java @@ -1,6 +1,7 @@ package com.exasol.adapter.request.parser; import static com.exasol.adapter.request.RequestJsonKeys.*; +import static java.util.Collections.unmodifiableMap; import java.math.BigDecimal; import java.util.*; @@ -22,13 +23,21 @@ * Parser for the JSON query AST. */ public final class PushdownSqlParser extends AbstractRequestParser { - private final List involvedTablesMetadata; + private final Map involvedTablesMetadataMap; private PushdownSqlParser(final List involvedTablesMetadata) { - this.involvedTablesMetadata = involvedTablesMetadata; + involvedTablesMetadataMap = buildInvolvedTablesMetadataMap(involvedTablesMetadata); } - private static ExaCharset charSetFromString(final String charset) { + private static Map buildInvolvedTablesMetadataMap(final List involvedTablesMetadata) { + final Map map = new HashMap<>((int) (involvedTablesMetadata.size() / 0.75f) + 1); + for (final TableMetadata involvedTableMeta : involvedTablesMetadata) { + map.put(involvedTableMeta.getName(), involvedTableMeta); + } + return unmodifiableMap(map); + } + + static ExaCharset charSetFromString(final String charset) { if (charset.equals("UTF8")) { return ExaCharset.UTF8; } else if (charset.equals("ASCII")) { @@ -141,17 +150,21 @@ private SqlGroupBy parseGroupBy( } } - private boolean hasAggregateFunction(final List nodesList) { - // Stack is less efficient than ArrayDeque, but ArrayDeque doesn't support null elements. - @java.lang.SuppressWarnings("java:S1149") - final Stack expressions = new Stack<>(); - expressions.addAll(nodesList); + static boolean hasAggregateFunction(final List nodesList) { + final Deque expressions = new ArrayDeque<>(); + for (final SqlNode node : nodesList) { + if (node != null) { + expressions.push(node); + } + } while (!expressions.isEmpty()) { final SqlNode expression = expressions.pop(); - if (expression != null) { - expressions.addAll(expression.getChildren()); - if (expression.getType().equals(SqlNodeType.FUNCTION_AGGREGATE)) { - return true; + if (expression.getType().equals(SqlNodeType.FUNCTION_AGGREGATE)) { + return true; + } + for (final SqlNode child : expression.getChildren()) { + if (child != null) { + expressions.push(child); } } } @@ -177,7 +190,7 @@ private SqlSelectList parseSelectList(final JsonArray selectList) { } } - private static IntervalType intervalTypeFromString(final String intervalType) { + static IntervalType intervalTypeFromString(final String intervalType) { if (intervalType.equals("DAY TO SECONDS")) { return IntervalType.DAY_TO_SECOND; } else if (intervalType.equals("YEAR TO MONTH")) { @@ -278,14 +291,6 @@ public SqlNode parseExpression(final JsonObject expression) { } } - private Map getInvolvedTablesMetadataMap() { - final Map tableMetadataMap = new HashMap<>(this.involvedTablesMetadata.size()); - for (final TableMetadata involvedTableMeta : this.involvedTablesMetadata) { - tableMetadataMap.put(involvedTableMeta.getName(), involvedTableMeta); - } - return tableMetadataMap; - } - private SqlColumn createColumn(final int index, final SqlTable table, final ColumnMetadata columnMetadata) { if (table.hasAlias()) { return new SqlColumn(index, columnMetadata, table.getName(), table.getAlias()); @@ -394,14 +399,13 @@ private SqlNode parseLiteralInterval(final JsonObject expression) { } @SuppressWarnings("java:S1192") // tableName is duplicated but that's ok since it's a parameter - private List collectAllInvolvedColumns(final SqlNode from) { + List collectAllInvolvedColumns(final SqlNode from) { final List involvedTables = collectInvolvedTables(from); - final Map tableMetadataMap = getInvolvedTablesMetadataMap(); final List selectListElements = new ArrayList<>(); for (final SqlTable table : involvedTables) { final String tableName = table.getName(); - if (tableMetadataMap.containsKey(tableName)) { - final List columns = tableMetadataMap.get(tableName).getColumns(); + if (involvedTablesMetadataMap.containsKey(tableName)) { + final List columns = involvedTablesMetadataMap.get(tableName).getColumns(); for (int i = 0, columnsSize = columns.size(); i < columnsSize; ++i) { selectListElements.add(createColumn(i, table, columns.get(i))); } @@ -414,29 +418,29 @@ private List collectAllInvolvedColumns(final SqlNode from) { return selectListElements; } - private DataType getHashtype(final JsonObject dataType) { + private static DataType getHashtype(final JsonObject dataType) { final int byteSize = dataType.getInt(BYTE_SIZE); return DataType.createHashtype(byteSize); } - private DataType getVarchar(final JsonObject dataType) { + private static DataType getVarchar(final JsonObject dataType) { final String charSet = dataType.getString(CHARACTER_SET, "UTF8"); return DataType.createVarChar(dataType.getInt(SIZE), charSetFromString(charSet)); } - private DataType getChar(final JsonObject dataType) { + private static DataType getChar(final JsonObject dataType) { final String charSet = dataType.getString(CHARACTER_SET, "UTF8"); return DataType.createChar(dataType.getInt(SIZE), charSetFromString(charSet)); } - private DataType getTimestamp(final JsonObject dataType) { + private static DataType getTimestamp(final JsonObject dataType) { final boolean withLocalTimezone = dataType.getBoolean(WITH_LOCAL_TIME_ZONE, false); final int precision = dataType.getInt(SchemaMetadataJsonConverter.TIMESTAMP_PRECISION_KEY, DataTypeParser.DEFAULT_TIMESTAMP_PRECISION); return DataType.createTimestamp(withLocalTimezone, precision); } - private DataType getInterval(final JsonObject dataType) { + private static DataType getInterval(final JsonObject dataType) { final int precision = dataType.getInt(PRECISION, 2); final IntervalType intervalType = intervalTypeFromString(dataType.getString(FROM_TO)); if (intervalType == IntervalType.DAY_TO_SECOND) { @@ -447,7 +451,7 @@ private DataType getInterval(final JsonObject dataType) { } } - private DataType getGeometry(final JsonObject dataType) { + private static DataType getGeometry(final JsonObject dataType) { final int srid = dataType.getInt(SRID); return DataType.createGeometry(srid); } @@ -458,8 +462,8 @@ private DataType getGeometry(final JsonObject dataType) { */ private List collectInvolvedTables(final SqlNode from) { final List involvedTables = new ArrayList<>(); - final Stack nodes = new Stack<>(); - nodes.add(from); + final Deque nodes = new ArrayDeque<>(); + nodes.push(from); while (!nodes.isEmpty()) { final SqlNode node = nodes.pop(); switch (node.getType()) { @@ -467,8 +471,8 @@ private List collectInvolvedTables(final SqlNode from) { involvedTables.add((SqlTable) node); break; case JOIN: - nodes.add(((SqlJoin) node).getRight()); - nodes.add(((SqlJoin) node).getLeft()); + nodes.push(((SqlJoin) node).getRight()); + nodes.push(((SqlJoin) node).getLeft()); break; default: throw new IllegalStateException(ExaError.messageBuilder("E-VSCOMJAVA-10") @@ -479,7 +483,7 @@ private List collectInvolvedTables(final SqlNode from) { return involvedTables; } - private DataType getDataType(final JsonObject dataType) { + static DataType getDataType(final JsonObject dataType) { final String typeName = dataType.getString(TYPE).toUpperCase(Locale.ROOT); switch (typeName) { case "DECIMAL": @@ -725,15 +729,14 @@ private static SqlNodeType fromTypeName(final String typeName) { } private TableMetadata findInvolvedTableMetadata(final String tableName) { - assert this.involvedTablesMetadata != null; - for (final TableMetadata tableMetadata : this.involvedTablesMetadata) { - if (tableMetadata.getName().equals(tableName)) { - return tableMetadata; - } + final TableMetadata tableMetadata = involvedTablesMetadataMap.get(tableName); + if (tableMetadata != null) { + return tableMetadata; } throw new IllegalStateException(ExaError.messageBuilder("E-VSCOMJAVA-14").message( "Could not find table metadata for involved table \"{{tableName|uq}}\". All involved tables: {{involvedTables}}") - .parameter("tableName", tableName).parameter("involvedTables", this.involvedTablesMetadata.toString()) + .parameter("tableName", tableName) + .parameter("involvedTables", this.involvedTablesMetadataMap.toString()) .toString()); } @@ -747,8 +750,10 @@ private ColumnMetadata findColumnMetadata(final String tableName, final String c throw new IllegalStateException(ExaError.messageBuilder("E-VSCOMJAVA-15").message( "Could not find column metadata for involved table \"{{tableName|uq}}\" and column \"{{columnName|uq}}\". " + "All involved tables: {{involvedTables}}.") - .parameter("tableName", tableName).parameter("columnName", columnName) - .parameter("involvedTables", this.involvedTablesMetadata.toString()).toString()); + .parameter("tableName", tableName) + .parameter("columnName", columnName) + .parameter("involvedTables", this.involvedTablesMetadataMap.toString()) + .toString()); } /** diff --git a/src/test/java/com/exasol/adapter/request/parser/PushDownSqlParserTest.java b/src/test/java/com/exasol/adapter/request/parser/PushDownSqlParserTest.java index f088421..8f7e9de 100644 --- a/src/test/java/com/exasol/adapter/request/parser/PushDownSqlParserTest.java +++ b/src/test/java/com/exasol/adapter/request/parser/PushDownSqlParserTest.java @@ -8,6 +8,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.io.IOException; import java.io.StringReader; @@ -437,6 +439,34 @@ void testParsePredicateLike() { () -> assertThat(pattern.getValue(), equalTo("a_d"))); } + @Test + void testParsePredicateLikeWithEscapeChar() { + final String sqlAsJson = "{" + + " \"type\" : \"predicate_like\", " + + " \"expression\" : { " + + " \"type\" : \"literal_string\", " + + " \"value\" : \"abcd\" " + + " }, " + + " \"pattern\" : { " + + " \"type\" : \"literal_string\", " + + " \"value\" : \"a!_d\" " + + " }, " + + " \"escapeChar\" : { " + + " \"type\" : \"literal_string\", " + + " \"value\" : \"!\" " + + " } " + + "}"; + final JsonObject jsonObject = createJsonObjectFromString(sqlAsJson); + final SqlPredicateLike sqlPredicateLike = (SqlPredicateLike) this.defaultParser.parseExpression(jsonObject); + final SqlLiteralString left = (SqlLiteralString) sqlPredicateLike.getLeft(); + final SqlLiteralString pattern = (SqlLiteralString) sqlPredicateLike.getPattern(); + final SqlLiteralString escapeChar = (SqlLiteralString) sqlPredicateLike.getEscapeChar(); + assertAll(() -> assertThat(sqlPredicateLike.getType(), equalTo(PREDICATE_LIKE)), + () -> assertThat(left.getValue(), equalTo("abcd")), // + () -> assertThat(pattern.getValue(), equalTo("a!_d")), // + () -> assertThat(escapeChar.getValue(), equalTo("!"))); + } + @Test void testParsePredicateLikeRegexp() { final String sqlAsJson = "{" // @@ -633,6 +663,61 @@ void testParseFunctionScalarCastToTimestamp() { assertThat(sqlFunctionScalarCast.getDataType().toString(), equalTo("TIMESTAMP(5)")); } + @Test + void testGetDataTypeDecimal() { + assertThat(getDataType("{" + + " \"type\" : \"DECIMAL\", " + + " \"precision\" : 31, " + + " \"scale\" : 41 " + + "}"), equalTo(createDecimal(31, 41))); + } + + @Test + void testGetDataTypeDouble() { + assertThat(getDataType("{" + + " \"type\" : \"DOUBLE\" " + + "}"), equalTo(DataType.createDouble())); + } + + @Test + void testGetDataTypeChar() { + assertThat(getDataType("{" + + " \"type\" : \"CHAR\", " + + " \"size\" : 22 " + + "}"), equalTo(DataType.createChar(22, UTF8))); + } + + @Test + void testGetDataTypeBoolean() { + assertThat(getDataType("{" + + " \"type\" : \"BOOLEAN\" " + + "}"), equalTo(DataType.createBool())); + } + + @Test + void testGetDataTypeDate() { + assertThat(getDataType("{" + + " \"type\" : \"DATE\" " + + "}"), equalTo(DataType.createDate())); + } + + @Test + void testGetDataTypeGeometry() { + assertThat(getDataType("{" + + " \"type\" : \"GEOMETRY\", " + + " \"srid\" : 42 " + + "}"), equalTo(DataType.createGeometry(42))); + } + + @Test + void testGetDataTypeUnsupportedType() { + final IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> getDataType("{" + + " \"type\" : \"UNSUPPORTED\" " + + "}")); + assertThat(exception.getMessage(), equalTo("E-VSCOMJAVA-11: Unsupported data type encountered: 'UNSUPPORTED'.")); + } + @Test void testParseFunctionScalarCast() { final String sqlAsJson = "{" // @@ -1155,11 +1240,13 @@ void testParseSelectWithHaving() { final PushdownSqlParser pushdownSqlParser = getCustomPushdownSqlParserWithTwoTables(); final SqlStatementSelect sqlStatementSelect = (SqlStatementSelect) pushdownSqlParser .parseExpression(jsonObject); - assertThat(sqlStatementSelect.getChildren().size(), equalTo(6)); - assertTrue(sqlStatementSelect.hasHaving()); - assertTrue(sqlStatementSelect.hasLimit()); - assertTrue(sqlStatementSelect.hasFilter()); - assertTrue(sqlStatementSelect.hasOrderBy()); + assertAll( + () -> assertThat(sqlStatementSelect.getChildren().size(), equalTo(6)), + () -> assertTrue(sqlStatementSelect.hasHaving()), + () -> assertFalse(sqlStatementSelect.hasGroupBy()), + () -> assertTrue(sqlStatementSelect.hasLimit()), + () -> assertTrue(sqlStatementSelect.hasFilter()), + () -> assertTrue(sqlStatementSelect.hasOrderBy())); } @Test @@ -1349,6 +1436,34 @@ void testParseSelectWithoutSelectListAndInvolvedTablesMetadata() { assertThat(exception.getMessage(), containsString("E-VSCOMJAVA-28")); } + @Test + void testCollectAllInvolvedColumnsWithMissingMetadata() { + final PushdownSqlParser parser = PushdownSqlParser.createWithTablesMetadata(Collections.emptyList()); + final SqlTable table = mock(SqlTable.class); + when(table.getType()).thenReturn(TABLE); + when(table.getName()).thenReturn("MISSING_TABLE"); + final IllegalStateException exception = assertThrows(IllegalStateException.class, + () -> parser.collectAllInvolvedColumns(table)); + assertThat(exception.getMessage(), equalTo("E-VSCOMJAVA-9: Unable to find metadata for table \"MISSING_TABLE\" during collecting involved columns.")); + } + + @Test + void testParserCopiesInvolvedTablesMetadata() { + final List tables = new ArrayList<>(); + tables.add(new TableMetadata("CUSTOMERS", "", List.of( + ColumnMetadata.builder().name("ID").adapterNotes("").type(createVarChar(200, UTF8)).build()), "")); + final PushdownSqlParser pushdownSqlParser = PushdownSqlParser.createWithTablesMetadata(tables); + tables.clear(); + final JsonObject jsonObject = createJsonObjectFromString("{" + + " \"type\" : \"table\", " + + " \"name\" : \"CUSTOMERS\" " + + "}"); + final SqlTable sqlTable = (SqlTable) pushdownSqlParser.parseExpression(jsonObject); + assertAll(() -> assertThat(sqlTable.getType(), equalTo(TABLE)), + () -> assertThat(sqlTable.getName(), equalTo("CUSTOMERS")), + () -> assertThat(sqlTable.getMetadata().getName(), equalTo("CUSTOMERS"))); + } + @Test void testParseInvalidExpressionType() { final String sqlAsJson = "{" // @@ -1427,4 +1542,41 @@ void testParseSelectWithSingleGroupAggregationWithAggregateFunction() { assertFalse(sqlStatementSelect.hasGroupBy()); assertThat(sqlStatementSelect.getChildren().size(), equalTo(2)); } + + @Test + void testHasAggregateFunctionIgnoresNullValues() { + final SqlFunctionAggregate aggregate = new SqlFunctionAggregate(AggregateFunction.SUM, + List.of(new SqlLiteralExactnumeric(BigDecimal.ONE)), false); + assertAll( + () -> assertThat(PushdownSqlParser.hasAggregateFunction(Collections.singletonList(null)), equalTo(false)), + () -> assertThat(PushdownSqlParser.hasAggregateFunction(Arrays.asList(null, aggregate)), equalTo(true))); + } + + @Test + void testCharSetFromStringAscii() { + assertThat(PushdownSqlParser.charSetFromString("ASCII"), equalTo(DataType.ExaCharset.ASCII)); + } + + @Test + void testCharSetFromStringUnsupportedCharset() { + final IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> PushdownSqlParser.charSetFromString("EBCDIC")); + assertThat(exception.getMessage(), equalTo("E-VSCOMJAVA-12: Unsupported charset encountered: 'EBCDIC'. Supported charsets are 'UTF8' and 'ASCII'.")); + } + + @Test + void testIntervalTypeFromStringYearToMonth() { + assertThat(PushdownSqlParser.intervalTypeFromString("YEAR TO MONTH"), equalTo(DataType.IntervalType.YEAR_TO_MONTH)); + } + + @Test + void testIntervalTypeFromStringUnsupportedInterval() { + final IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> PushdownSqlParser.intervalTypeFromString("HOUR")); + assertThat(exception.getMessage(), + equalTo("E-VSCOMJAVA-13: Unsupported interval data type encountered: 'HOUR'. Supported intervals are 'DAY TO SECONDS' and 'YEAR TO MONTH'.")); + } + + private DataType getDataType(final String dataTypeJson) { + return PushdownSqlParser.getDataType(createJsonObjectFromString(dataTypeJson)); + } }