From b949c75c292f53ef59a3cfedc3d462bf2d3d274f Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sat, 29 Aug 2026 16:54:46 -0700 Subject: [PATCH 1/2] Support PostgreSQL hex bytea literals Accept PostgreSQL hex-format BYTEA constants in both the infix `'\x0102'::bytea` form and the standard `CAST('\x0102' AS BYTEA)` spelling, normalizing them to Pinot's existing binary literal representation before either engine plans the query. The SQL-standard `X'0102'` form is unchanged. Only quoted constants are normalized. A dynamic STRING-to-BYTES conversion would have to be implemented per row in each engine, and the two would be easy to drift apart. PostgreSQL's legacy octal escape format stays unsupported. Parsing: - `InfixCast` appends the operator and target type to the enclosing expression list as a `SqlParserUtil.ToTreeListItem`, so precedence is resolved by `Expression2` the same way as the adjacent `SqlAtTimeZone` and Calcite's own Babel production. Collapsing the list eagerly would make `::` bind everything to its left, which breaks the most common shape: `WHERE bytesCol = '\x01'::bytea` would parse as `CAST(bytesCol = '\x01' AS BYTEA)`. - The `[]` and `.ident` postfix forms need no handling here; `Expression2` already owns those branches. Rewriting: - Only ASCII hex digits and ASCII whitespace are accepted. `Character.digit` and `Character.isWhitespace` also accept full-width and non-Latin forms, which PostgreSQL rejects. Whitespace is allowed between byte pairs but not inside one, matching PostgreSQL. - A `::` whose target type does not survive as a type spec (`col::bytea[1]`, where the item accessor binds tighter) is rejected rather than passed on as a malformed call. - Non-BYTEA `::` casts are rejected. This is a deliberate scope limit, not a technical one: `INFIX_CAST` is an ordinary `SqlCastOperator` of kind `CAST`, so `intCol::double` would plan like `CAST(intCol AS DOUBLE)`. Supporting the full operator means committing to `::` type-name semantics across both engines and is left to a follow-up. The grammar accepts `::` so that this rewriter can report a clear error instead of a parse failure. - Error messages name the offending type or expression and distinguish a malformed hex constant from a non-constant operand from an unsupported target. Tests cover scalar literals, BYTES array construction, bytea constants as the right operand of `=`, `<>`, `>`, `<=` and inside compound predicates, agreement between the `::`, `CAST` and `X'...'` spellings in both parse paths and both cases, ingested multi-value BYTES predicates, planner type inference, and runtime execution against H2. --- .../LiteralOnlyBrokerRequestTest.java | 50 ++++++- pinot-common/src/main/codegen/config.fmpp | 2 + .../src/main/codegen/includes/parserImpls.ftl | 19 +++ .../pinot/sql/parsers/CalciteSqlParser.java | 2 + .../sql/parsers/PostgreSqlCastRewriter.java | 141 ++++++++++++++++++ .../common/request/LiteralSerDeTest.java | 2 + .../utils/request/RequestUtilsTest.java | 6 + .../sql/parsers/CalciteSqlParserTest.java | 104 +++++++++++++ .../tests/custom/BytesMvTypeTest.java | 56 ++++--- .../tests/custom/BytesTypeTest.java | 21 +++ .../pinot/query/QueryCompilationTest.java | 11 ++ .../test/resources/queries/BinaryTypes.json | 27 ++-- 12 files changed, 401 insertions(+), 40 deletions(-) create mode 100644 pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java diff --git a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java index 5db658610dc8..fa3e4eca8766 100644 --- a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java +++ b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java @@ -97,12 +97,16 @@ public void testArrayLiteralBrokerRequestFromSQL() assertEquals((int[]) resultTable.getRows().get(0)[0], new int[]{1, 2}); assertEquals((String[]) resultTable.getRows().get(0)[1], new String[]{"one", "two"}); - brokerResponse = requestHandler.handleRequest("SELECT ARRAY[X'00', X'0102'] AS bytes"); - resultTable = brokerResponse.getResultTable(); - assertEquals(resultTable.getDataSchema().getColumnName(0), "bytes"); - assertEquals(resultTable.getDataSchema().getColumnDataType(0), DataSchema.ColumnDataType.BYTES_ARRAY); - assertEquals(resultTable.getRows().size(), 1); - assertEquals(resultTable.getRows().get(0), new Object[]{new String[]{"00", "0102"}}); + // The SQL-standard and PostgreSQL spellings must produce the same BYTES_ARRAY response. + for (String arrayLiteral : List.of("ARRAY[X'00', X'0102']", "ARRAY['\\x00'::bytea, '\\x0102'::bytea]", + "ARRAY[CAST('\\x00' AS BYTEA), CAST('\\x0102' AS BYTEA)]")) { + brokerResponse = requestHandler.handleRequest("SELECT " + arrayLiteral + " AS bytes"); + resultTable = brokerResponse.getResultTable(); + assertEquals(resultTable.getDataSchema().getColumnName(0), "bytes"); + assertEquals(resultTable.getDataSchema().getColumnDataType(0), DataSchema.ColumnDataType.BYTES_ARRAY); + assertEquals(resultTable.getRows().size(), 1); + assertEquals(resultTable.getRows().get(0), new Object[]{new String[]{"00", "0102"}}); + } brokerResponse = requestHandler.handleRequest( "SELECT ARRAYS_OVERLAP(ARRAY[X'00', X'0102'], ARRAY[X'03', X'0102']) AS overlaps"); @@ -111,6 +115,40 @@ public void testArrayLiteralBrokerRequestFromSQL() assertEquals(resultTable.getRows().get(0)[0], true); } + /// A scalar bytea constant must be answered by the literal-only path exactly like `X'...'`, in both spellings. + @Test + public void testScalarBytesLiteralBrokerRequestFromSQL() + throws Exception { + SingleConnectionBrokerRequestHandler requestHandler = + new SingleConnectionBrokerRequestHandler(new PinotConfiguration(), "testBrokerId", + new BrokerRequestIdGenerator(), null, ACCESS_CONTROL_FACTORY, null, null, null, null, + mock(ServerRoutingStatsManager.class), mock(FailureDetector.class), + ThreadAccountantUtils.getNoOpAccountant(), null, null); + + for (String literal : List.of("X'0102'", "'\\x0102'::bytea", "CAST('\\x0102' AS BYTEA)")) { + assertTrue(isLiteralOnlyQuery(CalciteSqlParser.compileToPinotQuery("SELECT " + literal))); + BrokerResponse brokerResponse = requestHandler.handleRequest("SELECT " + literal + " AS b"); + ResultTable resultTable = brokerResponse.getResultTable(); + assertTrue(brokerResponse.getExceptions().isEmpty(), literal); + assertEquals(resultTable.getDataSchema().getColumnDataType(0), DataSchema.ColumnDataType.BYTES, literal); + assertEquals(resultTable.getRows().get(0)[0], "0102", literal); + } + + // An empty bytea constant is legal in PostgreSQL and yields zero-length BYTES. + BrokerResponse brokerResponse = requestHandler.handleRequest("SELECT '\\x'::bytea AS b"); + assertEquals(brokerResponse.getResultTable().getDataSchema().getColumnDataType(0), + DataSchema.ColumnDataType.BYTES); + assertEquals(brokerResponse.getResultTable().getRows().get(0)[0], ""); + + // Constant folding over bytea elements still happens at parse time, so the query stays literal-only. + String folded = "SELECT ARRAY_LENGTH(ARRAY['\\x00'::bytea, '\\x0102'::bytea]) AS n"; + assertTrue(isLiteralOnlyQuery(CalciteSqlParser.compileToPinotQuery(folded))); + brokerResponse = requestHandler.handleRequest(folded); + assertEquals(brokerResponse.getResultTable().getDataSchema().getColumnDataType(0), + DataSchema.ColumnDataType.INT); + assertEquals(brokerResponse.getResultTable().getRows().get(0)[0], 2); + } + @Test public void testLiteralOnlyTransformBrokerRequestFromSQL() { assertTrue(isLiteralOnlyQuery(CalciteSqlParser.compileToPinotQuery("SELECT now()"))); diff --git a/pinot-common/src/main/codegen/config.fmpp b/pinot-common/src/main/codegen/config.fmpp index 32b716473868..7e3493d183ba 100644 --- a/pinot-common/src/main/codegen/config.fmpp +++ b/pinot-common/src/main/codegen/config.fmpp @@ -654,11 +654,13 @@ data: { # Binary operators tokens. # Example: "< INFIX_CAST: \"::\" >". binaryOperatorsTokens: [ + "< INFIX_CAST: \"::\" >" ] # Binary operators initialization. # Example: "InfixCast". extraBinaryExpressions: [ + "InfixCast" "SqlAtTimeZone" ] diff --git a/pinot-common/src/main/codegen/includes/parserImpls.ftl b/pinot-common/src/main/codegen/includes/parserImpls.ftl index 06070cff9352..3fe417fed38e 100644 --- a/pinot-common/src/main/codegen/includes/parserImpls.ftl +++ b/pinot-common/src/main/codegen/includes/parserImpls.ftl @@ -88,6 +88,25 @@ void SqlAtTimeZone(List list, ExprContext exprContext, Span s) : } } +/// Parses the PostgreSQL infix `::` cast operator. The operator and its target type are appended to the enclosing +/// expression list so `SqlParserUtil.toTree` applies the standard precedence rules, exactly like `SqlAtTimeZone` +/// above; collapsing the list here instead would make `::` bind the whole expression to its left. PostgreSQL BYTEA +/// literals are normalized after parsing so both query engines receive the same binary literal representation as +/// SQL `X'...'`. +void InfixCast(List list, ExprContext exprContext, Span s) : +{ + final SqlDataTypeSpec dataType; +} +{ + { + checkNonQueryExpression(exprContext); + } + dataType = DataType() { + list.add(new SqlParserUtil.ToTreeListItem(SqlLibraryOperators.INFIX_CAST, s.pos())); + list.add(dataType); + } +} + SqlNode SqlPhysicalExplain() : { SqlNode stmt; diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java index d50f0f1e8dd2..8dd32037971c 100644 --- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java @@ -140,6 +140,7 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql) try (StringReader inStream = new StringReader(sql)) { SqlParserImpl sqlParser = newSqlParser(inStream); SqlNodeList sqlNodeList = sqlParser.parseSqlStmtList(); + sqlNodeList = (SqlNodeList) PostgreSqlCastRewriter.rewrite(sqlNodeList); // Extract OPTION statements from sql. SqlNodeAndOptions sqlNodeAndOptions = extractSqlNodeAndOptions(sqlNodeList); // add legacy OPTIONS keyword-based options @@ -669,6 +670,7 @@ public static Expression compileToExpression(String expression) { try (StringReader inStream = new StringReader(expression)) { SqlParserImpl sqlParser = newSqlParser(inStream); sqlNode = sqlParser.parseSqlExpressionEof(); + sqlNode = PostgreSqlCastRewriter.rewrite(sqlNode); } catch (Throwable e) { throw new SqlCompilationException("Caught exception while parsing expression: " + expression, e); } diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java new file mode 100644 index 000000000000..76505c7c94f0 --- /dev/null +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java @@ -0,0 +1,141 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.sql.parsers; + +import java.util.List; +import org.apache.calcite.sql.SqlBinaryStringLiteral; +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlCharStringLiteral; +import org.apache.calcite.sql.SqlDataTypeSpec; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.fun.SqlLibraryOperators; +import org.apache.calcite.sql.util.SqlShuttle; + + +/// Normalizes PostgreSQL hex-format BYTEA constants (`'\x0102'::bytea` and `CAST('\x0102' AS BYTEA)`) to Calcite +/// binary literals, so both query engines see the same representation they would get from SQL `X'0102'`. Only quoted +/// constants are normalized: a dynamic `STRING`-to-`BYTES` conversion would have to be implemented per row in each +/// engine, and the two would be easy to drift apart. +/// +/// Every other use of the PostgreSQL `::` operator is rejected here. That is a deliberate scope limit, not a +/// technical one — `SqlLibraryOperators.INFIX_CAST` is an ordinary `SqlCastOperator` of kind +/// [org.apache.calcite.sql.SqlKind#CAST], so `intCol::double` would in fact plan and run like `CAST(intCol AS +/// DOUBLE)`. Supporting the full operator means committing to `::` type-name semantics across both engines and is +/// left to a follow-up; until then the grammar accepts `::` only so that this rewriter can give a clear error +/// instead of a parse failure. +/// +/// Stateless and safe to share; `rewrite` uses a single immutable instance. +final class PostgreSqlCastRewriter extends SqlShuttle { + private static final PostgreSqlCastRewriter INSTANCE = new PostgreSqlCastRewriter(); + private static final String HEX_PREFIX = "\\x"; + private static final String BYTEA_TYPE_NAME = "BYTEA"; + + private PostgreSqlCastRewriter() { + } + + static SqlNode rewrite(SqlNode sqlNode) { + return sqlNode.accept(INSTANCE); + } + + @Override + public SqlNode visit(SqlCall call) { + SqlNode visitedNode = super.visit(call); + if (!(visitedNode instanceof SqlCall)) { + return visitedNode; + } + SqlCall visitedCall = (SqlCall) visitedNode; + List operands = visitedCall.getOperandList(); + boolean infixCast = visitedCall.getOperator() == SqlLibraryOperators.INFIX_CAST; + if (visitedCall.getKind() != SqlKind.CAST || operands.size() != 2 + || !(operands.get(1) instanceof SqlDataTypeSpec)) { + if (infixCast) { + // The target type did not survive as a type spec, e.g. `col::bytea[1]`, where the item accessor binds + // tighter than `::`. Reject it here rather than letting the malformed call reach the planner. + throw new SqlCompilationException("Unsupported PostgreSQL :: cast target in '" + visitedCall + + "'. Note that [] binds tighter than ::, so write CAST( AS ) instead"); + } + return visitedCall; + } + + SqlDataTypeSpec targetType = (SqlDataTypeSpec) operands.get(1); + boolean bytea = targetType.getTypeName().isSimple() + && targetType.getTypeName().getSimple().equalsIgnoreCase(BYTEA_TYPE_NAME); + if (!bytea) { + if (infixCast) { + throw new SqlCompilationException("PostgreSQL-style :: casts are supported only for BYTEA hex constants, " + + "not for target type '" + targetType.getTypeName() + "'. Use CAST( AS ) instead"); + } + return visitedCall; + } + + SqlNode source = operands.get(0); + if (source instanceof SqlBinaryStringLiteral) { + return source; + } + if (!(source instanceof SqlCharStringLiteral)) { + throw new SqlCompilationException("BYTEA casts are supported only for quoted hex constants such as " + + "'\\x0102', not for the expression '" + source + "'"); + } + String value = ((SqlCharStringLiteral) source).getValueAs(String.class); + if (!value.startsWith(HEX_PREFIX)) { + throw invalidByteaLiteral(value); + } + return SqlBinaryStringLiteral.createBinaryString(normalizeHex(value), visitedCall.getParserPosition()); + } + + /// Decodes the digits after the leading `\x`. PostgreSQL allows whitespace between byte pairs but not inside one. + private static String normalizeHex(String literal) { + String value = literal.substring(HEX_PREFIX.length()); + StringBuilder hex = new StringBuilder(value.length()); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (isAsciiWhitespace(c)) { + if ((hex.length() & 1) != 0) { + throw invalidByteaLiteral(literal); + } + } else if (isAsciiHexDigit(c)) { + hex.append(c); + } else { + throw invalidByteaLiteral(literal); + } + } + if ((hex.length() & 1) != 0) { + throw invalidByteaLiteral(literal); + } + return hex.toString(); + } + + /// Deliberately not `Character.digit(c, 16)`: that also accepts full-width and non-Latin digits such as `Ａ` and + /// `١`, which PostgreSQL rejects. + private static boolean isAsciiHexDigit(char c) { + return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); + } + + /// Deliberately not `Character.isWhitespace(c)`, for the same reason as [#isAsciiHexDigit]: PostgreSQL only skips + /// ASCII whitespace between byte pairs, so `'\x01 02'` is an error rather than `0x0102`. + private static boolean isAsciiWhitespace(char c) { + return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\f' || c == 0x0B; + } + + private static SqlCompilationException invalidByteaLiteral(String value) { + return new SqlCompilationException("Invalid PostgreSQL BYTEA hex constant '" + value + "': it must begin with " + + "\\x and contain complete hexadecimal byte pairs, optionally separated by ASCII whitespace"); + } +} diff --git a/pinot-common/src/test/java/org/apache/pinot/common/request/LiteralSerDeTest.java b/pinot-common/src/test/java/org/apache/pinot/common/request/LiteralSerDeTest.java index 1056f9a11ba6..5727cb145ed2 100644 --- a/pinot-common/src/test/java/org/apache/pinot/common/request/LiteralSerDeTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/common/request/LiteralSerDeTest.java @@ -93,7 +93,9 @@ public void testBytesArrayDeepCopy() { public void testSingleStageQueryUsesNativeBytesArrayLiteral() throws TException { for (String sql : List.of("SELECT ARRAY[X'00', X'0102'] FROM myTable", + "SELECT ARRAY['\\x00'::bytea, CAST('\\x0102' AS BYTEA)] FROM myTable", "SELECT id FROM myTable WHERE ARRAYS_OVERLAP(bytesMV, ARRAY[X'01'])", + "SELECT id FROM myTable WHERE ARRAYS_OVERLAP(bytesMV, ARRAY['\\x01'::bytea])", "SELECT ARRAY[X'02'], COUNT(*) FROM myTable GROUP BY ARRAY[X'02']", "SELECT COUNT(*) FROM myTable HAVING ARRAYS_OVERLAP(ARRAYAGG(bytesColumn, 'BYTES'), ARRAY[X'03'])", "SELECT id FROM myTable " diff --git a/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java b/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java index e9acb5336de8..a65148b37901 100644 --- a/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java @@ -126,6 +126,12 @@ public void testBytesArrayLiteralRepresentations() { assertTrue(expression.getLiteral().isSetBytesArrayValue()); assertEquals(RequestUtils.getBytesArrayValue(expression.getLiteral()), expected); + expression = CalciteSqlParser.compileToPinotQuery( + "SELECT ARRAY['\\x00'::bytea, CAST('\\xDEADBEEF' AS BYTEA)] FROM myTable").getSelectList().get(0); + assertTrue(expression.isSetLiteral()); + assertTrue(expression.getLiteral().isSetBytesArrayValue()); + assertEquals(RequestUtils.getBytesArrayValue(expression.getLiteral()), expected); + expression = CalciteSqlParser.compileToPinotQuery( "SELECT ARRAYS_OVERLAP(ARRAY[X'00', X'0102'], ARRAY[X'03', X'0102'])").getSelectList().get(0); assertTrue(expression.isSetLiteral()); diff --git a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java index dec47000efba..7f86d57360a0 100644 --- a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java @@ -19,6 +19,7 @@ package org.apache.pinot.sql.parsers; import java.util.List; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.pinot.common.request.Expression; import org.apache.pinot.common.request.Function; import org.apache.pinot.common.request.PinotQuery; @@ -57,6 +58,108 @@ public void resetLegacyUnescaping() { RequestUtils.setUseLegacyLiteralUnescaping(true); } + @Test + public void testPostgreSqlByteaHexLiterals() { + PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery("SELECT '\\x0102'::bytea"); + assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getBinaryValue(), new byte[]{1, 2}); + + pinotQuery = CalciteSqlParser.compileToPinotQuery("SELECT CAST('\\xDe Ad Be Ef' AS BYTEA)"); + assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getBinaryValue(), + new byte[]{(byte) 0xde, (byte) 0xad, (byte) 0xbe, (byte) 0xef}); + + pinotQuery = CalciteSqlParser.compileToPinotQuery("SELECT '\\x'::ByTeA"); + assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getBinaryValue(), new byte[0]); + + Expression expression = CalciteSqlParser.compileToExpression("'\\x0102'::bytea"); + assertEquals(expression.getLiteral().getBinaryValue(), new byte[]{1, 2}); + } + + /// `::` must bind only to the literal on its left, not to the whole expression accumulated so far, so that a bytea + /// constant can be used as the right operand of a comparison. + @Test(dataProvider = "binaryOperatorsTakingByteaLiterals") + public void testPostgreSqlByteaLiteralBindsTighterThanBinaryOperators(String predicate, String expectedOperator) { + PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery( + "SELECT id FROM myTable WHERE bytesColumn " + predicate + " '\\x0102'::bytea"); + Function filter = pinotQuery.getFilterExpression().getFunctionCall(); + assertEquals(filter.getOperator(), expectedOperator); + assertEquals(filter.getOperands().get(0).getIdentifier().getName(), "bytesColumn"); + assertEquals(filter.getOperands().get(1).getLiteral().getBinaryValue(), new byte[]{1, 2}); + } + + @DataProvider + public static Object[][] binaryOperatorsTakingByteaLiterals() { + return new Object[][]{ + {"=", "EQUALS"}, + {"<>", "NOT_EQUALS"}, + {">", "GREATER_THAN"}, + {"<=", "LESS_THAN_OR_EQUAL"} + }; + } + + @Test + public void testPostgreSqlByteaLiteralInCompoundPredicate() { + PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery( + "SELECT id FROM myTable WHERE id = 1 AND bytesColumn = '\\x0102'::bytea"); + Function and = pinotQuery.getFilterExpression().getFunctionCall(); + assertEquals(and.getOperator(), "AND"); + Function byteaEquals = and.getOperands().get(1).getFunctionCall(); + assertEquals(byteaEquals.getOperands().get(0).getIdentifier().getName(), "bytesColumn"); + assertEquals(byteaEquals.getOperands().get(1).getLiteral().getBinaryValue(), new byte[]{1, 2}); + } + + /// The infix and standard cast spellings must normalize to the same binary literal wherever they appear. + @Test + public void testPostgreSqlByteaLiteralMatchesStandardCastSpelling() { + for (String bytea : List.of("'\\x0102'::bytea", "'\\x0102' :: ByTeA", "CAST('\\x0102' AS BYTEA)", + "cast('\\x0102' as bytea)", "X'0102'")) { + PinotQuery pinotQuery = + CalciteSqlParser.compileToPinotQuery("SELECT id FROM myTable WHERE bytesColumn = " + bytea); + assertEquals(pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(1).getLiteral() + .getBinaryValue(), new byte[]{1, 2}, bytea); + assertEquals(CalciteSqlParser.compileToExpression(bytea).getLiteral().getBinaryValue(), new byte[]{1, 2}, + bytea); + } + } + + @Test(dataProvider = "invalidPostgreSqlByteaLiterals") + public void testInvalidPostgreSqlByteaHexLiterals(String sql, String expectedMessageFragment) { + SqlCompilationException e = + expectThrows(SqlCompilationException.class, () -> CalciteSqlParser.compileToPinotQuery(sql)); + assertTrue(ExceptionUtils.getStackTrace(e).contains(expectedMessageFragment), + "Expected <" + expectedMessageFragment + "> for " + sql + " but got: " + e.getMessage()); + } + + private static final String INVALID_CONSTANT = "Invalid PostgreSQL BYTEA hex constant"; + private static final String NOT_A_CONSTANT = "BYTEA casts are supported only for quoted hex constants"; + + @DataProvider + public static Object[][] invalidPostgreSqlByteaLiterals() { + return new Object[][]{ + // Malformed hex constants. + {"SELECT '\\x0'::bytea", INVALID_CONSTANT}, + {"SELECT '\\x0g'::bytea", INVALID_CONSTANT}, + {"SELECT '\\x0 1'::bytea", INVALID_CONSTANT}, + {"SELECT '0102'::bytea", INVALID_CONSTANT}, + // Full-width and non-Latin digits are hex digits to Character.digit but not to PostgreSQL. + {"SELECT '\\x\uFF21\uFF22'::bytea", INVALID_CONSTANT}, + {"SELECT '\\x\u0660\u0661'::bytea", INVALID_CONSTANT}, + // Non-ASCII whitespace is whitespace to Character.isWhitespace but not to PostgreSQL. + {"SELECT '\\x01\u205F02'::bytea", INVALID_CONSTANT}, + + // BYTEA casts of something that is not a constant. + {"SELECT bytesColumn::bytea FROM myTable", NOT_A_CONSTANT}, + {"SELECT CAST(bytesColumn AS BYTEA) FROM myTable", NOT_A_CONSTANT}, + {"SELECT id FROM myTable WHERE id = 1 AND bytesColumn::bytea = X'01'", NOT_A_CONSTANT}, + + // `::` to a target type other than BYTEA. + {"SELECT 1::int", "not for target type 'INTEGER'"}, + {"SELECT bytesColumn::varchar FROM myTable", "not for target type 'VARCHAR'"}, + + // The item accessor binds tighter than `::`, so the target type does not survive as a type spec. + {"SELECT bytesColumn::bytea[1] FROM myTable", "Unsupported PostgreSQL :: cast target"} + }; + } + @Test public void testIdentifierLength() { String tableName = extendIdentifierToMaxLength("exampleTable"); @@ -102,6 +205,7 @@ public static Object[][] nonReservedKeywords() { new Object[]{"string"}, new Object[]{"varchar"}, new Object[]{"bytes"}, + new Object[]{"bytea"}, new Object[]{"binary"}, new Object[]{"varbinary"}, new Object[]{"variant"}, diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesMvTypeTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesMvTypeTest.java index 25b440b931f8..c0e95d86c0df 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesMvTypeTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesMvTypeTest.java @@ -188,17 +188,19 @@ public void testSelectWithMvColumn(boolean useMultiStageQueryEngine) public void testBytesArrayLiteral(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine); - String arrayLiteral = "ARRAY[X'07', X'0708', X'07090A']"; - for (boolean withFrom : new boolean[]{true, false}) { - String query = withFrom ? String.format("SELECT %s FROM %s WHERE %s = 7 LIMIT 1", arrayLiteral, - getTableName(), ID_COLUMN) : "SELECT " + arrayLiteral; - JsonNode result = postQuery(query).get("resultTable"); - assertEquals(result.get("dataSchema").get("columnDataTypes").get(0).asText(), "BYTES_ARRAY"); - JsonNode values = result.get("rows").get(0).get(0); - assertEquals(values.size(), MV_LENGTH); - assertEquals(values.get(0).asText(), "07"); - assertEquals(values.get(1).asText(), "0708"); - assertEquals(values.get(2).asText(), "07090a"); + for (String arrayLiteral : List.of("ARRAY[X'07', X'0708', X'07090A']", + "ARRAY['\\x07'::bytea, '\\x0708'::bytea, '\\x07090A'::bytea]")) { + for (boolean withFrom : new boolean[]{true, false}) { + String query = withFrom ? String.format("SELECT %s FROM %s WHERE %s = 7 LIMIT 1", arrayLiteral, + getTableName(), ID_COLUMN) : "SELECT " + arrayLiteral; + JsonNode result = postQuery(query).get("resultTable"); + assertEquals(result.get("dataSchema").get("columnDataTypes").get(0).asText(), "BYTES_ARRAY"); + JsonNode values = result.get("rows").get(0).get(0); + assertEquals(values.size(), MV_LENGTH); + assertEquals(values.get(0).asText(), "07"); + assertEquals(values.get(1).asText(), "0708"); + assertEquals(values.get(2).asText(), "07090a"); + } } } @@ -219,15 +221,18 @@ public void testArraysOverlapWithLiteral(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine); for (String mvCol : MV_COLUMNS) { - String positiveQuery = String.format( - "SELECT COUNT(*) FROM %s WHERE ARRAYS_OVERLAP(%s, ARRAY[X'07'])", getTableName(), mvCol); - JsonNode rows = postQuery(positiveQuery).get("resultTable").get("rows"); - assertEquals(rows.get(0).get(0).asLong(), 1L); - - String negativeQuery = String.format( - "SELECT COUNT(*) FROM %s WHERE ARRAYS_OVERLAP(%s, ARRAY[X'FF'])", getTableName(), mvCol); - rows = postQuery(negativeQuery).get("resultTable").get("rows"); - assertEquals(rows.get(0).get(0).asLong(), 0L); + for (String[] literals : List.of(new String[]{"X'07'", "X'FF'"}, + new String[]{"'\\x07'::bytea", "'\\xFF'::bytea"})) { + String positiveQuery = String.format( + "SELECT COUNT(*) FROM %s WHERE ARRAYS_OVERLAP(%s, ARRAY[%s])", getTableName(), mvCol, literals[0]); + JsonNode rows = postQuery(positiveQuery).get("resultTable").get("rows"); + assertEquals(rows.get(0).get(0).asLong(), 1L); + + String negativeQuery = String.format( + "SELECT COUNT(*) FROM %s WHERE ARRAYS_OVERLAP(%s, ARRAY[%s])", getTableName(), mvCol, literals[1]); + rows = postQuery(negativeQuery).get("resultTable").get("rows"); + assertEquals(rows.get(0).get(0).asLong(), 0L); + } } } @@ -235,11 +240,14 @@ public void testArraysOverlapWithLiteral(boolean useMultiStageQueryEngine) public void testArraysOverlapWithLiterals(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine); - JsonNode result = postQuery( - "SELECT ARRAYS_OVERLAP(ARRAY[X'00', X'0102'], ARRAY[X'03', X'0102'])").get("resultTable"); - assertTrue(result.get("rows").get(0).get(0).asBoolean()); + for (String overlapping : List.of("ARRAYS_OVERLAP(ARRAY[X'00', X'0102'], ARRAY[X'03', X'0102'])", + "ARRAYS_OVERLAP(ARRAY[CAST('\\x00' AS BYTEA), CAST('\\x0102' AS BYTEA)], " + + "ARRAY['\\x03'::bytea, '\\x0102'::bytea])")) { + JsonNode result = postQuery("SELECT " + overlapping).get("resultTable"); + assertTrue(result.get("rows").get(0).get(0).asBoolean()); + } - result = postQuery( + JsonNode result = postQuery( "SELECT ARRAYS_OVERLAP(ARRAY[X'00', X'0102'], ARRAY[X'03', X'04'])").get("resultTable"); assertFalse(result.get("rows").get(0).get(0).asBoolean()); } diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesTypeTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesTypeTest.java index ff392d5a691a..af9b3ca0fb0e 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesTypeTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/BytesTypeTest.java @@ -307,4 +307,25 @@ public void testStringAndBytesInPredicate(boolean useMultiStageQueryEngine) Assert.assertEquals(rows.get(i).get(0).asLong(), NUM_TOTAL_DOCS); } } + + /// Regression coverage for the PostgreSQL `::` cast binding to the whole expression on its left instead of just + /// the literal, which made a bytea constant unusable as the right operand of a comparison. + @Test(dataProvider = "useBothQueryEngines") + public void testPostgreSqlByteaLiteralAsPredicateOperand(boolean useMultiStageQueryEngine) + throws Exception { + setUseMultiStageQueryEngine(useMultiStageQueryEngine); + for (String byteaLiteral : List.of("'\\x" + FIXED_HEX_STRING_VALUE + "'::bytea", + "CAST('\\x" + FIXED_HEX_STRING_VALUE + "' AS BYTEA)", "X'" + FIXED_HEX_STRING_VALUE + "'")) { + String query = String.format("SELECT count(*) FROM %s WHERE %s = %s", getTableName(), FIXED_BYTES, + byteaLiteral); + JsonNode rows = postQuery(query).get("resultTable").get("rows"); + Assert.assertEquals(rows.get(0).get(0).asLong(), NUM_TOTAL_DOCS, query); + + // Same literal on the right of a compound predicate, where the accumulated expression list is non-empty. + query = String.format("SELECT count(*) FROM %s WHERE %s IS NOT NULL AND %s = %s", getTableName(), FIXED_BYTES, + FIXED_BYTES, byteaLiteral); + rows = postQuery(query).get("resultTable").get("rows"); + Assert.assertEquals(rows.get(0).get(0).asLong(), NUM_TOTAL_DOCS, query); + } + } } diff --git a/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java b/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java index 30f74e5d60cd..e8fcc64fddd5 100644 --- a/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java +++ b/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java @@ -143,6 +143,17 @@ public void testUuidPolymorphicInputTypeInference() { } } + @Test + public void testPostgreSqlByteaLiteralTypeInference() { + RelDataType rowType = _queryEnvironment.compile( + "SELECT '\\x0102'::bytea, ARRAY['\\x00'::bytea, CAST('\\x0102' AS BYTEA)] FROM a") + .getRelRoot().validatedRowType; + assertEquals(rowType.getFieldList().get(0).getType().getSqlTypeName(), SqlTypeName.BINARY); + RelDataType arrayType = rowType.getFieldList().get(1).getType(); + assertEquals(arrayType.getSqlTypeName(), SqlTypeName.ARRAY); + assertEquals(arrayType.getComponentType().getSqlTypeName(), SqlTypeName.VARBINARY); + } + /// `jsonPath` must resolve to a literal, but the operand type checker deliberately does not demand a literal /// `SqlNode` in that position: operand checking runs before `PinotEvaluateLiteralRule` folds constant /// expressions, so an argument such as `CONCAT('$.', 'foo')` folds to a literal and plans and executes diff --git a/pinot-query-runtime/src/test/resources/queries/BinaryTypes.json b/pinot-query-runtime/src/test/resources/queries/BinaryTypes.json index 6ce66cea1657..33bb20b4e71f 100644 --- a/pinot-query-runtime/src/test/resources/queries/BinaryTypes.json +++ b/pinot-query-runtime/src/test/resources/queries/BinaryTypes.json @@ -18,21 +18,28 @@ }, { "psql": "8.4.1", - "ignored": true, - "comments": [ - "looks like we don't support constants, this is treated as a normal string", - "we also require using calcite syntax for byte strings to parse, which is x'deadbeef' instead", - "of postgres syntax which is '\\xdeadbeaf'" - ], - "description": "bytea hex constants", - "sql": "SELECT x'DEADBEEF', data from {bytea}" + "description": "PostgreSQL bytea hex constants", + "sql": "SELECT '\\xDEADBEEF'::bytea, data FROM {bytea}", + "h2Sql": "SELECT X'DEADBEEF', data FROM {bytea}" + }, + { + "psql": "8.4.1", + "description": "PostgreSQL bytea hex constant as a comparison operand", + "sql": "SELECT data FROM {bytea} WHERE data = '\\xDEADBEEF'::bytea", + "h2Sql": "SELECT data FROM {bytea} WHERE data = X'DEADBEEF'" + }, + { + "psql": "8.4.1", + "description": "PostgreSQL bytea hex constant in a compound predicate", + "sql": "SELECT data FROM {bytea} WHERE data <> '\\x00'::bytea AND data = CAST('\\xDEADBEEF' AS BYTEA)", + "h2Sql": "SELECT data FROM {bytea} WHERE data <> X'00' AND data = X'DEADBEEF'" }, { "psql": "8.4.2", "ignored": true, "comments": [ - "in order to specify bytea constants using escape syntax, we need type casting", - "which we don't support. also, calcite doesn't support the escape syntax for bytes" + "PostgreSQL's legacy octal bytea escape syntax remains unsupported", + "only PostgreSQL hex-format bytea literals are normalized to Pinot bytes" ], "description": "bytea escape constants", "sql": "SELECT CAST('\\046' AS BINARY), data from {bytea}" From 6e2b49bf8c4086c5e585aad9983b1d820654b1fc Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Mon, 21 Sep 2026 15:52:10 -0700 Subject: [PATCH 2/2] Rewrite bytea constants in place to preserve Pinot statement nodes The rewriter extended SqlShuttle, which is copy-on-write: replacing a constant rebuilt every ancestor through SqlOperator.createCall, and that does not preserve Pinot's own statement classes. EXPLAIN IMPLEMENTATION PLAN FOR with a bytea constant came back as a plain SqlExplain, so the broker silently returned the logical plan, and CREATE MATERIALIZED VIEW came back as a SqlBasicCall classified as DQL, so the DDL compiler rejected it. Replace only the constant's direct parent, through SqlCall.setOperand or SqlNodeList.set, so every other node keeps its identity and class. Every node under which Pinot's grammar accepts an arbitrary expression supports this; the exceptions are positions the grammar restricts, such as SqlOrderBy's numeric offset and fetch, and Pinot's own statement nodes, which hold queries, identifiers and literals. A parent that cannot be updated in place is rejected with a clear error instead of being rebuilt. Record the position of the `::` token itself in InfixCast, as BinaryRowOperator does. s.pos() started at the token before the enclosing expression, so the cast and its rewritten literal claimed spans such as `WHERE b = '\x01'::bytea`, which misplaced validator error highlights. Also from review: - Name hexToBytes() as the per-row alternative when a BYTEA cast has a non-constant operand. - Say that only the hex format is supported when a constant lacks the \x prefix. PostgreSQL reads '0102'::bytea as escape format (four bytes). - Run the rewrite outside compileToExpression's catch-all so its messages reach the caller instead of only its cause. - Link the BYTEA-only scope of `::` to the follow-up issue, #19628. - Test X'...'::bytea and CAST(X'...' AS BYTEA), the case-sensitive \x prefix, an IN list, the literal's source span, node class and statement type for EXPLAIN and CREATE MATERIALIZED VIEW (including a nested subquery), the physical plan returned by EXPLAIN IMPLEMENTATION PLAN, and the stored definition of a materialized view whose body contains a bytea constant. --- .../src/main/codegen/includes/parserImpls.ftl | 8 +- .../pinot/sql/parsers/CalciteSqlParser.java | 3 +- .../sql/parsers/PostgreSqlCastRewriter.java | 98 +++++++++--- .../sql/parsers/CalciteSqlParserTest.java | 143 +++++++++++++++++- .../pinot/query/QueryCompilationTest.java | 15 ++ .../DdlCompilerMaterializedViewTest.java | 22 +++ 6 files changed, 261 insertions(+), 28 deletions(-) diff --git a/pinot-common/src/main/codegen/includes/parserImpls.ftl b/pinot-common/src/main/codegen/includes/parserImpls.ftl index 3fe417fed38e..161fad10ca3c 100644 --- a/pinot-common/src/main/codegen/includes/parserImpls.ftl +++ b/pinot-common/src/main/codegen/includes/parserImpls.ftl @@ -93,16 +93,22 @@ void SqlAtTimeZone(List list, ExprContext exprContext, Span s) : /// above; collapsing the list here instead would make `::` bind the whole expression to its left. PostgreSQL BYTEA /// literals are normalized after parsing so both query engines receive the same binary literal representation as /// SQL `X'...'`. +/// +/// The operator records the position of the `::` token itself, as `BinaryRowOperator` does; `toTree` then widens it +/// to cover the operands. `s.pos()` would instead start at the token before the enclosing expression, so the cast +/// and the literal it is rewritten to would claim a span such as `WHERE b = '\x01'::bytea`. void InfixCast(List list, ExprContext exprContext, Span s) : { + final SqlParserPos pos; final SqlDataTypeSpec dataType; } { { checkNonQueryExpression(exprContext); + pos = getPos(); } dataType = DataType() { - list.add(new SqlParserUtil.ToTreeListItem(SqlLibraryOperators.INFIX_CAST, s.pos())); + list.add(new SqlParserUtil.ToTreeListItem(SqlLibraryOperators.INFIX_CAST, pos)); list.add(dataType); } } diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java index 8dd32037971c..0d85fb9b03de 100644 --- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java @@ -670,10 +670,11 @@ public static Expression compileToExpression(String expression) { try (StringReader inStream = new StringReader(expression)) { SqlParserImpl sqlParser = newSqlParser(inStream); sqlNode = sqlParser.parseSqlExpressionEof(); - sqlNode = PostgreSqlCastRewriter.rewrite(sqlNode); } catch (Throwable e) { throw new SqlCompilationException("Caught exception while parsing expression: " + expression, e); } + // Outside the try: the rewriter already throws SqlCompilationException, and wrapping it would drop its message. + sqlNode = PostgreSqlCastRewriter.rewrite(sqlNode); return toExpression(sqlNode); } diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java index 76505c7c94f0..ed6974e0e785 100644 --- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/PostgreSqlCastRewriter.java @@ -25,6 +25,7 @@ import org.apache.calcite.sql.SqlDataTypeSpec; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlNodeList; import org.apache.calcite.sql.fun.SqlLibraryOperators; import org.apache.calcite.sql.util.SqlShuttle; @@ -38,10 +39,22 @@ /// technical one — `SqlLibraryOperators.INFIX_CAST` is an ordinary `SqlCastOperator` of kind /// [org.apache.calcite.sql.SqlKind#CAST], so `intCol::double` would in fact plan and run like `CAST(intCol AS /// DOUBLE)`. Supporting the full operator means committing to `::` type-name semantics across both engines and is -/// left to a follow-up; until then the grammar accepts `::` only so that this rewriter can give a clear error -/// instead of a parse failure. +/// tracked in https://github.com/apache/pinot/issues/19628; until then the grammar accepts `::` only so that this +/// rewriter can give a clear error instead of a parse failure. Rejecting now keeps that open: relaxing a rejection +/// later is backward compatible, while narrowing an accepted type would not be. /// -/// Stateless and safe to share; `rewrite` uses a single immutable instance. +/// The tree is rewritten in place. A plain [SqlShuttle] is copy-on-write: it rebuilds every ancestor of a replaced +/// node through `SqlOperator.createCall`, and that does not preserve Pinot's own statement classes. A +/// `SqlPhysicalExplain` comes back as a plain `SqlExplain` and a `SqlPinotCreateMaterializedView` as a `SqlBasicCall`, +/// which silently changes how the statement is planned or classified. Here only a constant's direct parent is +/// modified, through `SqlCall.setOperand` or `SqlNodeList.set`, so every other node keeps its identity and class. +/// Every node under which Pinot's grammar accepts an arbitrary expression supports that. The exceptions are positions +/// the grammar restricts: `SqlOrderBy` holds its offset and fetch directly, but they must be numeric, and Pinot's own +/// statement nodes hold queries, identifiers and literals rather than bare expressions. A parent that cannot be +/// updated in place is rejected with a clear error rather than rebuilt. +/// +/// Stateless and safe to share across threads. It mutates only the tree passed to `rewrite`, which must be owned by +/// the caller, such as a freshly parsed statement. final class PostgreSqlCastRewriter extends SqlShuttle { private static final PostgreSqlCastRewriter INSTANCE = new PostgreSqlCastRewriter(); private static final String HEX_PREFIX = "\\x"; @@ -50,28 +63,73 @@ final class PostgreSqlCastRewriter extends SqlShuttle { private PostgreSqlCastRewriter() { } + /// Rewrites `sqlNode` in place and returns it, or returns the replacement literal when `sqlNode` is itself a bytea + /// constant, as for an expression parsed on its own. static SqlNode rewrite(SqlNode sqlNode) { return sqlNode.accept(INSTANCE); } + @Override + public SqlNode visit(SqlNodeList nodeList) { + for (int i = 0; i < nodeList.size(); i++) { + SqlNode node = nodeList.get(i); + if (node != null) { + SqlNode rewritten = node.accept(this); + if (rewritten != node) { + try { + nodeList.set(i, rewritten); + } catch (UnsupportedOperationException e) { + throw unsupportedParent("an immutable node list", e); + } + } + } + } + return nodeList; + } + @Override public SqlNode visit(SqlCall call) { - SqlNode visitedNode = super.visit(call); - if (!(visitedNode instanceof SqlCall)) { - return visitedNode; + List operands = call.getOperandList(); + for (int i = 0; i < operands.size(); i++) { + SqlNode operand = operands.get(i); + if (operand != null) { + SqlNode rewritten = operand.accept(this); + if (rewritten != operand) { + setOperand(call, i, rewritten); + } + } + } + return rewriteCast(call); + } + + private static void setOperand(SqlCall parent, int index, SqlNode replacement) { + try { + parent.setOperand(index, replacement); + } catch (UnsupportedOperationException e) { + throw unsupportedParent(parent.getKind().toString(), e); } - SqlCall visitedCall = (SqlCall) visitedNode; - List operands = visitedCall.getOperandList(); - boolean infixCast = visitedCall.getOperator() == SqlLibraryOperators.INFIX_CAST; - if (visitedCall.getKind() != SqlKind.CAST || operands.size() != 2 - || !(operands.get(1) instanceof SqlDataTypeSpec)) { + } + + /// Not reachable from Pinot's grammar today. Fails with a clear error if a future node holds a bytea constant where + /// it cannot be replaced in place, rather than falling back to rebuilding the node. + private static SqlCompilationException unsupportedParent(String parent, UnsupportedOperationException cause) { + return new SqlCompilationException( + "PostgreSQL BYTEA constants are not supported inside " + parent + "; use X'...' instead", cause); + } + + /// Returns the binary literal that a supported bytea constant normalizes to, or `call` itself when it is not a cast + /// this rewriter handles. Throws for every other use of the PostgreSQL `::` operator. + private static SqlNode rewriteCast(SqlCall call) { + List operands = call.getOperandList(); + boolean infixCast = call.getOperator() == SqlLibraryOperators.INFIX_CAST; + if (call.getKind() != SqlKind.CAST || operands.size() != 2 || !(operands.get(1) instanceof SqlDataTypeSpec)) { if (infixCast) { // The target type did not survive as a type spec, e.g. `col::bytea[1]`, where the item accessor binds // tighter than `::`. Reject it here rather than letting the malformed call reach the planner. - throw new SqlCompilationException("Unsupported PostgreSQL :: cast target in '" + visitedCall + throw new SqlCompilationException("Unsupported PostgreSQL :: cast target in '" + call + "'. Note that [] binds tighter than ::, so write CAST( AS ) instead"); } - return visitedCall; + return call; } SqlDataTypeSpec targetType = (SqlDataTypeSpec) operands.get(1); @@ -82,7 +140,7 @@ public SqlNode visit(SqlCall call) { throw new SqlCompilationException("PostgreSQL-style :: casts are supported only for BYTEA hex constants, " + "not for target type '" + targetType.getTypeName() + "'. Use CAST( AS ) instead"); } - return visitedCall; + return call; } SqlNode source = operands.get(0); @@ -91,13 +149,14 @@ public SqlNode visit(SqlCall call) { } if (!(source instanceof SqlCharStringLiteral)) { throw new SqlCompilationException("BYTEA casts are supported only for quoted hex constants such as " - + "'\\x0102', not for the expression '" + source + "'"); + + "'\\x0102', not for the expression '" + source + "'. To convert hex strings per row, use " + + "hexToBytes(), which takes plain hexadecimal digits without the \\x prefix"); } String value = ((SqlCharStringLiteral) source).getValueAs(String.class); if (!value.startsWith(HEX_PREFIX)) { throw invalidByteaLiteral(value); } - return SqlBinaryStringLiteral.createBinaryString(normalizeHex(value), visitedCall.getParserPosition()); + return SqlBinaryStringLiteral.createBinaryString(normalizeHex(value), call.getParserPosition()); } /// Decodes the digits after the leading `\x`. PostgreSQL allows whitespace between byte pairs but not inside one. @@ -129,13 +188,14 @@ private static boolean isAsciiHexDigit(char c) { } /// Deliberately not `Character.isWhitespace(c)`, for the same reason as [#isAsciiHexDigit]: PostgreSQL only skips - /// ASCII whitespace between byte pairs, so `'\x01 02'` is an error rather than `0x0102`. + /// ASCII whitespace between byte pairs, so a non-ASCII space such as U+205F between them is an error. private static boolean isAsciiWhitespace(char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\f' || c == 0x0B; } private static SqlCompilationException invalidByteaLiteral(String value) { - return new SqlCompilationException("Invalid PostgreSQL BYTEA hex constant '" + value + "': it must begin with " - + "\\x and contain complete hexadecimal byte pairs, optionally separated by ASCII whitespace"); + return new SqlCompilationException("Invalid PostgreSQL BYTEA constant '" + value + "': only the hex format is " + + "supported, so it must begin with \\x and contain complete hexadecimal byte pairs, optionally separated by " + + "ASCII whitespace"); } } diff --git a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java index 7f86d57360a0..552aeea604c4 100644 --- a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java @@ -18,18 +18,29 @@ */ package org.apache.pinot.sql.parsers; +import java.io.StringReader; import java.util.List; -import org.apache.commons.lang3.exception.ExceptionUtils; +import org.apache.calcite.sql.SqlBinaryStringLiteral; +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlExplain; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlNodeList; +import org.apache.calcite.sql.SqlOrderBy; +import org.apache.calcite.sql.SqlSelect; +import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.pinot.common.request.Expression; import org.apache.pinot.common.request.Function; import org.apache.pinot.common.request.PinotQuery; import org.apache.pinot.common.utils.request.RequestUtils; +import org.apache.pinot.sql.parsers.parser.SqlPhysicalExplain; +import org.apache.pinot.sql.parsers.parser.SqlPinotCreateMaterializedView; import org.testng.annotations.AfterMethod; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import static org.apache.pinot.sql.parsers.CalciteSqlParser.CALCITE_SQL_PARSER_IDENTIFIER_MAX_LENGTH; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertThrows; import static org.testng.Assert.assertTrue; import static org.testng.Assert.expectThrows; @@ -107,11 +118,12 @@ public void testPostgreSqlByteaLiteralInCompoundPredicate() { assertEquals(byteaEquals.getOperands().get(1).getLiteral().getBinaryValue(), new byte[]{1, 2}); } - /// The infix and standard cast spellings must normalize to the same binary literal wherever they appear. + /// The infix and standard cast spellings must normalize to the same binary literal wherever they appear, including + /// when the source is already a binary literal. @Test public void testPostgreSqlByteaLiteralMatchesStandardCastSpelling() { for (String bytea : List.of("'\\x0102'::bytea", "'\\x0102' :: ByTeA", "CAST('\\x0102' AS BYTEA)", - "cast('\\x0102' as bytea)", "X'0102'")) { + "cast('\\x0102' as bytea)", "X'0102'", "X'0102'::bytea", "CAST(X'0102' AS BYTEA)")) { PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery("SELECT id FROM myTable WHERE bytesColumn = " + bytea); assertEquals(pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(1).getLiteral() @@ -121,15 +133,129 @@ public void testPostgreSqlByteaLiteralMatchesStandardCastSpelling() { } } + /// Regression coverage for rebuilding Pinot's own statement nodes. A copy-on-write rewrite turned a + /// `SqlPhysicalExplain` into a plain `SqlExplain`, which silently returned the logical plan, and a + /// `SqlPinotCreateMaterializedView` into a `SqlBasicCall` classified as DQL. The statement must keep the class and + /// type it has with the equivalent `X'...'` literal, and the constant must still be normalized inside it. + @Test(dataProvider = "statementsContainingByteaLiterals") + public void testPostgreSqlByteaLiteralPreservesStatementNode(String sql, Class expectedClass, + PinotSqlType expectedType) { + for (String bytea : List.of("X'01'", "'\\x01'::bytea", "CAST('\\x01' AS BYTEA)")) { + String statement = sql.replace("?", bytea); + SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(statement); + SqlNode sqlNode = sqlNodeAndOptions.getSqlNode(); + assertEquals(sqlNode.getClass(), expectedClass, statement); + assertEquals(sqlNodeAndOptions.getSqlType(), expectedType, statement); + String unparsed = sqlNode.toString(); + assertTrue(unparsed.contains("X'01'"), statement + " unparsed as " + unparsed); + assertFalse(unparsed.toUpperCase().contains("BYTEA"), statement + " unparsed as " + unparsed); + } + } + + @DataProvider + public static Object[][] statementsContainingByteaLiterals() { + return new Object[][]{ + {"EXPLAIN IMPLEMENTATION PLAN FOR SELECT a FROM t WHERE b = ?", SqlPhysicalExplain.class, PinotSqlType.DQL}, + {"EXPLAIN PLAN FOR SELECT a FROM t WHERE b = ?", SqlExplain.class, PinotSqlType.DQL}, + {"CREATE MATERIALIZED VIEW mv AS SELECT a FROM t WHERE b = ?", SqlPinotCreateMaterializedView.class, + PinotSqlType.DDL}, + {"CREATE MATERIALIZED VIEW mv AS SELECT a FROM t WHERE a IN (SELECT a FROM u WHERE b = ?)", + SqlPinotCreateMaterializedView.class, PinotSqlType.DDL}, + {"SELECT a FROM t WHERE b = ? ORDER BY a LIMIT 5", SqlOrderBy.class, PinotSqlType.DQL} + }; + } + + /// A node that cannot replace an operand in place must fail with a clear error rather than be rebuilt with a + /// different class. Pinot's grammar never puts a bytea constant directly under such a node, so the tree is built + /// by hand. + @Test + public void testPostgreSqlByteaLiteralUnderNodeWithoutSetOperandIsRejected() + throws Exception { + SqlNode query = CalciteSqlParser.newSqlParser(new StringReader("SELECT a FROM t")).parseSqlStmtEof(); + SqlNode bytea = CalciteSqlParser.newSqlParser(new StringReader("'\\x01'::bytea")).parseSqlExpressionEof(); + SqlOrderBy orderBy = new SqlOrderBy(SqlParserPos.ZERO, query, SqlNodeList.EMPTY, bytea, null); + SqlCompilationException e = + expectThrows(SqlCompilationException.class, () -> PostgreSqlCastRewriter.rewrite(orderBy)); + assertTrue(e.getMessage().contains("not supported inside ORDER_BY"), e.getMessage()); + } + + /// Same guard for a node list that cannot be updated in place, such as one wrapping an immutable list. + @Test + public void testPostgreSqlByteaLiteralInImmutableNodeListIsRejected() + throws Exception { + SqlNode bytea = CalciteSqlParser.newSqlParser(new StringReader("'\\x01'::bytea")).parseSqlExpressionEof(); + SqlNodeList immutable = SqlNodeList.of(SqlParserPos.ZERO, List.of(bytea)); + SqlCompilationException e = + expectThrows(SqlCompilationException.class, () -> PostgreSqlCastRewriter.rewrite(immutable)); + assertTrue(e.getMessage().contains("not supported inside an immutable node list"), e.getMessage()); + } + + private static final String BYTEA_CONSTANT = "'\\x01'::bytea"; + + /// The literal a bytea constant is rewritten to must span exactly that constant, so validation errors point at it. + /// Recording the enclosing expression's start instead made it claim, e.g., `WHERE b = '\x01'::bytea`. + @Test + public void testPostgreSqlByteaLiteralKeepsItsSourcePosition() + throws Exception { + String sql = "SELECT a FROM t WHERE b = " + BYTEA_CONSTANT; + SqlSelect select = (SqlSelect) PostgreSqlCastRewriter.rewrite( + CalciteSqlParser.newSqlParser(new StringReader(sql)).parseSqlStmtEof()); + assertSpansByteaConstant(((SqlCall) select.getWhere()).operand(1), sql); + + sql = "SELECT a FROM t WHERE c LIKE 'x' AND b = " + BYTEA_CONSTANT; + select = (SqlSelect) PostgreSqlCastRewriter.rewrite( + CalciteSqlParser.newSqlParser(new StringReader(sql)).parseSqlStmtEof()); + assertSpansByteaConstant(((SqlCall) ((SqlCall) select.getWhere()).operand(1)).operand(1), sql); + + sql = "a || " + BYTEA_CONSTANT; + SqlCall concat = (SqlCall) PostgreSqlCastRewriter.rewrite( + CalciteSqlParser.newSqlParser(new StringReader(sql)).parseSqlExpressionEof()); + assertSpansByteaConstant(concat.operand(1), sql); + } + + private static void assertSpansByteaConstant(SqlNode literal, String sql) { + assertTrue(literal instanceof SqlBinaryStringLiteral, sql + " -> " + literal); + int start = sql.indexOf(BYTEA_CONSTANT) + 1; + SqlParserPos pos = literal.getParserPosition(); + assertEquals(pos.getLineNum(), 1, sql); + assertEquals(pos.getColumnNum(), start, sql); + assertEquals(pos.getEndColumnNum(), start + BYTEA_CONSTANT.length() - 1, sql); + } + + /// An IN list is a node list reached through the filter rather than the select list. + @Test + public void testPostgreSqlByteaLiteralInInList() { + PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery( + "SELECT id FROM myTable WHERE bytesColumn IN ('\\x01'::bytea, X'02', CAST('\\x03' AS BYTEA))"); + Function in = pinotQuery.getFilterExpression().getFunctionCall(); + assertEquals(in.getOperator(), "IN"); + assertEquals(in.getOperands().get(0).getIdentifier().getName(), "bytesColumn"); + assertEquals(in.getOperands().get(1).getLiteral().getBinaryValue(), new byte[]{1}); + assertEquals(in.getOperands().get(2).getLiteral().getBinaryValue(), new byte[]{2}); + assertEquals(in.getOperands().get(3).getLiteral().getBinaryValue(), new byte[]{3}); + } + + /// The expression parse path must surface the rewriter's own message rather than only a generic wrapper. + @Test + public void testPostgreSqlByteaErrorsSurfaceFromCompileToExpression() { + SqlCompilationException e = + expectThrows(SqlCompilationException.class, () -> CalciteSqlParser.compileToExpression("bytesColumn::bytea")); + assertTrue(e.getMessage().contains(NOT_A_CONSTANT), e.getMessage()); + assertTrue(e.getMessage().contains("hexToBytes()"), e.getMessage()); + + e = expectThrows(SqlCompilationException.class, () -> CalciteSqlParser.compileToExpression("'\\x0'::bytea")); + assertTrue(e.getMessage().contains(INVALID_CONSTANT), e.getMessage()); + } + @Test(dataProvider = "invalidPostgreSqlByteaLiterals") public void testInvalidPostgreSqlByteaHexLiterals(String sql, String expectedMessageFragment) { SqlCompilationException e = expectThrows(SqlCompilationException.class, () -> CalciteSqlParser.compileToPinotQuery(sql)); - assertTrue(ExceptionUtils.getStackTrace(e).contains(expectedMessageFragment), + assertTrue(e.getMessage().contains(expectedMessageFragment), "Expected <" + expectedMessageFragment + "> for " + sql + " but got: " + e.getMessage()); } - private static final String INVALID_CONSTANT = "Invalid PostgreSQL BYTEA hex constant"; + private static final String INVALID_CONSTANT = "Invalid PostgreSQL BYTEA constant"; private static final String NOT_A_CONSTANT = "BYTEA casts are supported only for quoted hex constants"; @DataProvider @@ -139,7 +265,10 @@ public static Object[][] invalidPostgreSqlByteaLiterals() { {"SELECT '\\x0'::bytea", INVALID_CONSTANT}, {"SELECT '\\x0g'::bytea", INVALID_CONSTANT}, {"SELECT '\\x0 1'::bytea", INVALID_CONSTANT}, - {"SELECT '0102'::bytea", INVALID_CONSTANT}, + // PostgreSQL reads this as escape format (four bytes), which is not supported. + {"SELECT '0102'::bytea", "only the hex format is supported"}, + // The \x prefix is case-sensitive, as in PostgreSQL. + {"SELECT '\\X0102'::bytea", INVALID_CONSTANT}, // Full-width and non-Latin digits are hex digits to Character.digit but not to PostgreSQL. {"SELECT '\\x\uFF21\uFF22'::bytea", INVALID_CONSTANT}, {"SELECT '\\x\u0660\u0661'::bytea", INVALID_CONSTANT}, @@ -148,7 +277,7 @@ public static Object[][] invalidPostgreSqlByteaLiterals() { // BYTEA casts of something that is not a constant. {"SELECT bytesColumn::bytea FROM myTable", NOT_A_CONSTANT}, - {"SELECT CAST(bytesColumn AS BYTEA) FROM myTable", NOT_A_CONSTANT}, + {"SELECT CAST(bytesColumn AS BYTEA) FROM myTable", "use hexToBytes()"}, {"SELECT id FROM myTable WHERE id = 1 AND bytesColumn::bytea = X'01'", NOT_A_CONSTANT}, // `::` to a target type other than BYTEA. diff --git a/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java b/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java index e8fcc64fddd5..714d39839443 100644 --- a/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java +++ b/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java @@ -154,6 +154,21 @@ public void testPostgreSqlByteaLiteralTypeInference() { assertEquals(arrayType.getComponentType().getSqlTypeName(), SqlTypeName.VARBINARY); } + /// A bytea constant must not change which plan EXPLAIN returns. Rewriting it used to rebuild the statement node, so + /// `EXPLAIN IMPLEMENTATION PLAN` lost its physical-plan marker and silently returned the logical plan instead. + @Test + public void testPostgreSqlByteaLiteralKeepsPhysicalExplain() { + long requestId = RANDOM_REQUEST_ID_GEN.nextLong(); + String expected = + _queryEnvironment.explainQuery("EXPLAIN IMPLEMENTATION PLAN FOR SELECT col1, X'01' FROM a", requestId); + assertTrue(expected.contains("MAIL_RECEIVE"), expected); + for (String bytea : List.of("'\\x01'::bytea", "CAST('\\x01' AS BYTEA)")) { + String explain = _queryEnvironment.explainQuery( + "EXPLAIN IMPLEMENTATION PLAN FOR SELECT col1, " + bytea + " FROM a", requestId); + assertEquals(explain, expected, bytea); + } + } + /// `jsonPath` must resolve to a literal, but the operand type checker deliberately does not demand a literal /// `SqlNode` in that position: operand checking runs before `PinotEvaluateLiteralRule` folds constant /// expressions, so an argument such as `CONCAT('$.', 'foo')` folds to a literal and plans and executes diff --git a/pinot-sql-ddl/src/test/java/org/apache/pinot/sql/ddl/compile/DdlCompilerMaterializedViewTest.java b/pinot-sql-ddl/src/test/java/org/apache/pinot/sql/ddl/compile/DdlCompilerMaterializedViewTest.java index 801041baa94a..10b88f3d94c1 100644 --- a/pinot-sql-ddl/src/test/java/org/apache/pinot/sql/ddl/compile/DdlCompilerMaterializedViewTest.java +++ b/pinot-sql-ddl/src/test/java/org/apache/pinot/sql/ddl/compile/DdlCompilerMaterializedViewTest.java @@ -18,6 +18,7 @@ */ package org.apache.pinot.sql.ddl.compile; +import java.util.List; import java.util.Map; import org.apache.pinot.spi.config.table.TableConfig; import org.apache.pinot.spi.config.table.TableType; @@ -423,6 +424,27 @@ public void definedSqlPreservesUserOriginalText() { "Select Carrier /* hot column */, ts From src"); } + /// A PostgreSQL bytea constant in the view body must leave the statement a materialized view (rewriting it once + /// rebuilt the statement as a plain call, which was then classified as a query), and the stored text must be + /// exactly what the user typed, since it is sliced from the union of every node's parser position. + @Test + public void definedSqlWithPostgreSqlByteaConstant() { + for (String bytea : List.of("'\\x01'::bytea", "CAST('\\x01' AS BYTEA)")) { + String query = "SELECT ts, carrier FROM src WHERE payload = " + bytea + " AND carrier <> 'x'"; + CompiledCreateMaterializedView c = compileMaterializedView( + "CREATE MATERIALIZED VIEW mv (" + + " ts TIMESTAMP DATETIME FORMAT '1:MILLISECONDS:TIMESTAMP' GRANULARITY '1:DAYS'," + + " carrier STRING" + + ")" + + " REFRESH EVERY 1 DAY" + + " PROPERTIES ('timeColumnName' = 'ts', 'bucketTimePeriod' = '1d')" + + " AS " + query); + assertEquals(c.getOperation(), DdlOperation.CREATE_MATERIALIZED_VIEW, bytea); + assertEquals(materializedViewTaskConfig(c.getTableConfig()).get(MaterializedViewTask.DEFINED_SQL_KEY), query, + bytea); + } + } + @Test public void definedSqlAcrossMultipleLinesPreserved() { String sql =