Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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()")));
Expand Down
2 changes: 2 additions & 0 deletions pinot-common/src/main/codegen/config.fmpp
Original file line number Diff line number Diff line change
Expand Up @@ -654,11 +654,13 @@ data: {
# Binary operators tokens.
# Example: "< INFIX_CAST: \"::\" >".
binaryOperatorsTokens: [
"< INFIX_CAST: \"::\" >"
]

# Binary operators initialization.
# Example: "InfixCast".
extraBinaryExpressions: [
"InfixCast"
"SqlAtTimeZone"
]

Expand Down
25 changes: 25 additions & 0 deletions pinot-common/src/main/codegen/includes/parserImpls.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,31 @@ void SqlAtTimeZone(List<Object> 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'...'`.
///
/// 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<Object> list, ExprContext exprContext, Span s) :
{
final SqlParserPos pos;
final SqlDataTypeSpec dataType;
}
{
<INFIX_CAST> {
checkNonQueryExpression(exprContext);
pos = getPos();
}
dataType = DataType() {
list.add(new SqlParserUtil.ToTreeListItem(SqlLibraryOperators.INFIX_CAST, pos));
list.add(dataType);
}
}

SqlNode SqlPhysicalExplain() :
{
SqlNode stmt;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs the shuttle over the whole statement list, so it also walks Pinot's own statement nodes.

SqlShuttle rebuilds every ancestor of a changed node with operator.createCall(...), and that call returns the operator's own node type. SqlPhysicalExplain inherits SqlExplain.OPERATOR, whose createCall hardcodes new SqlExplain(...). SqlPinotCreateMaterializedView uses a bare SqlSpecialOperator with no createCall override, so it rebuilds as SqlBasicCall. One bytea literal anywhere inside such a statement replaces the statement node with a different class.

I ran both cases against this branch:

EXPLAIN IMPLEMENTATION PLAN FOR SELECT a FROM t WHERE b = X'01'
  -> SqlPhysicalExplain

EXPLAIN IMPLEMENTATION PLAN FOR SELECT a FROM t WHERE b = '\x01'::bytea
  -> org.apache.calcite.sql.SqlExplain

QueryEnvironment:967 picks the physical plan with explain instanceof SqlPhysicalExplain. After the downgrade that test is false, so the broker returns the logical plan. The user gets no error.

CREATE MATERIALIZED VIEW mv AS SELECT a FROM t WHERE b = X'01'
  -> SqlPinotCreateMaterializedView, sqlType=DDL

CREATE MATERIALIZED VIEW mv AS SELECT a FROM t WHERE b = '\x01'::bytea
  -> SqlBasicCall, sqlType=DQL

extractSqlNodeAndOptions matches on the node class, so the statement becomes DQL and the DDL path never runs.

CAST('\x01' AS BYTEA) reproduces both, so :: is not needed to hit this. A bytea literal in a nested subquery of the view body triggers it too.

Two ways to correct this:

  1. Classify the statement first, then rewrite only the query node. For SqlExplain, rewrite operand 0 in place with setOperand.
  2. Give each custom node's operator a createCall override that rebuilds the concrete class, and give SqlPhysicalExplain its own OPERATOR.

Option 1 is smaller. Option 2 also protects these nodes from the next shuttle someone adds.

Please add regression tests that assert the node class and PinotSqlType for EXPLAIN IMPLEMENTATION PLAN FOR and for CREATE MATERIALIZED VIEW with a bytea literal. The current tests cover only a plain SELECT, so both defects pass CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This was a real bug, and your repro matched what I saw: SqlPhysicalExplain → SqlExplain, and SqlPinotCreateMaterializedView/DDL → SqlBasicCall/DQL, including with a nested subquery in the view body.

Fixed in 6e2b49b with a variant of option 1 that doesn't need to know the statement types. The rewriter no longer rebuilds anything: it replaces only the constant's direct parent in place (SqlCall.setOperand / SqlNodeList.set), so every other node keeps its identity and class. I preferred this to classifying first because 13 Pinot node types subclass SqlCall, and any of them that holds a query would have hit the same bug. It is also smaller than option 2.

For it to be safe, every node that can directly hold a bytea constant has to support setOperand. I checked Calcite 1.42 and the grammar:

  • Every node under which Pinot's grammar accepts an arbitrary expression supports it: SqlBasicCall, SqlNodeList, SqlSelect, SqlCase, SqlJoin, SqlWindow, SqlExplain, SqlWith, and the rest.
  • The nodes that don't only appear where the grammar is restricted: SqlOrderBy offset/fetch (numeric only), SqlHint, SqlTableRef, and star EXCLUDE/REPLACE (disabled).
  • Pinot's own nodes hold queries, identifiers and literals. The one that holds an expression, SqlPinotColumnDeclaration's DEFAULT, only accepts Literal().

If a future node does hold a constant somewhere it can't be replaced, the rewriter throws a SqlCompilationException naming the node instead of rebuilding it.

Regression tests, each of which fails against the previous copy-on-write rewriter:

  • CalciteSqlParserTest.testPostgreSqlByteaLiteralPreservesStatementNode checks the node class and PinotSqlType for EXPLAIN IMPLEMENTATION PLAN FOR, EXPLAIN PLAN FOR, and CREATE MATERIALIZED VIEW, with the constant both directly in the body and in a nested subquery. Each case runs with '\x01'::bytea, CAST('\x01' AS BYTEA) and X'01', and asserts the constant was still normalized.
  • QueryCompilationTest.testPostgreSqlByteaLiteralKeepsPhysicalExplain checks the user-visible symptom: the EXPLAIN IMPLEMENTATION PLAN output must equal the X'01' physical plan. Against the old code it gets the logical plan back.
  • DdlCompilerMaterializedViewTest.definedSqlWithPostgreSqlByteaConstant compiles a real materialized view end to end and asserts the stored definedSQL. Against the old code, the DDL compiler rejects it as an unsupported statement.

// Extract OPTION statements from sql.
SqlNodeAndOptions sqlNodeAndOptions = extractSqlNodeAndOptions(sqlNodeList);
// add legacy OPTIONS keyword-based options
Expand Down Expand Up @@ -672,6 +673,8 @@ public static Expression compileToExpression(String expression) {
} 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);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
/**
* 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.SqlNodeList;
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
/// 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.
///
/// 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";
private static final String BYTEA_TYPE_NAME = "BYTEA";

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) {
List<SqlNode> 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);
}
}

/// 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<SqlNode> 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 '" + call
+ "'. Note that [] binds tighter than ::, so write CAST(<expr> AS <type>) instead");
}
return call;
}

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(<expr> AS <type>) instead");
}
return call;
}

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 + "'. To convert hex strings per row, use "
+ "hexToBytes(<expr>), 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), call.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 `A` 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 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 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");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Loading
Loading