github-actions[bot] commented on code in PR #65984:
URL: https://github.com/apache/doris/pull/65984#discussion_r3643026245
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/NereidsParser.java:
##########
@@ -399,6 +411,23 @@ public static Map<Integer, ParserRuleContext>
getHintMap(String sql, CommonToken
return selectHintMap;
}
+ private static Map<Integer, ParserRuleContext> getTableHintMap(
+ CommonTokenStream tokenStream, Map<Integer, ParserRuleContext>
selectHintMap) {
+ Map<Integer, ParserRuleContext> tableHintMap = Maps.newHashMap();
+ Token previousDefaultToken = null;
+ for (Token token : tokenStream.getTokens()) {
+ ParserRuleContext hintContext =
selectHintMap.get(token.getStartIndex());
+ if (hintContext != null && previousDefaultToken != null) {
+ // Table hints are hidden tokens, so bind them to the closest
visible token on their left.
+ tableHintMap.put(previousDefaultToken.getTokenIndex(),
hintContext);
Review Comment:
[P2] Preserve all hints at the same table boundary
Adjacent hidden comments have no default-channel token between them, so both
are keyed to the same `previousDefaultToken` and the later `put` overwrites the
earlier context. Thus `t /*+ OPTIONS('scan.snapshot-id'='1') */ /*+
OPTIONS('scan.mode'='from-snapshot') */` silently loses the snapshot option
instead of triggering the intended one-OPTIONS-per-table error. Store all
contexts per anchor in source order (or reject the second one) and cover
separate adjacent comments.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2815,6 +2827,51 @@ public LogicalPlan visitTableName(TableNameContext ctx) {
return plan;
}
+ private TableScanParams visitTableOptionsHint(TableNameContext ctx) {
+ List<ParserRuleContext> hintContexts = Lists.newArrayList();
+ ParserRuleContext afterTableName =
+
tableHintMap.get(ctx.multipartIdentifier().getStop().getTokenIndex());
+ if (afterTableName != null) {
+ hintContexts.add(afterTableName);
+ }
+ ParserRuleContext afterRelation =
tableHintMap.get(ctx.getStop().getTokenIndex());
+ if (afterRelation != null && afterRelation != afterTableName) {
+ hintContexts.add(afterRelation);
+ }
+
+ Map<String, String> options = null;
+ for (ParserRuleContext hintContext : hintContexts) {
+ SelectHintContext selectHintContext = (SelectHintContext)
hintContext;
+ for (HintStatementContext hintStatement :
selectHintContext.hintStatements) {
+ if (hintStatement.hintName() == null
+ ||
!"options".equalsIgnoreCase(hintStatement.hintName().getText())) {
+ continue;
+ }
+ if (options != null) {
+ throw new AnalysisException("Only one OPTIONS hint is
allowed for each table");
+ }
+ options = Maps.newLinkedHashMap();
+ for (HintAssignmentContext kv : hintStatement.parameters) {
+ if (kv.key == null || (kv.constantValue == null &&
kv.identifierValue == null)) {
+ throw new AnalysisException("OPTIONS hint requires
key-value assignments");
+ }
+ String key = visitIdentifierOrText(kv.key);
+ String value;
+ if (kv.constantValue != null) {
+ Literal literal = (Literal) visit(kv.constantValue);
+ value = literal.toLegacyLiteral().getStringValue();
+ } else {
+ value = kv.identifierValue.getText();
+ }
+ options.put(key, value);
Review Comment:
[P2] Reject duplicate option keys
This unconditional `put` makes `/*+ OPTIONS('scan.snapshot-id'='1',
'scan.snapshot-id'='2') */` silently execute with snapshot 2; Paimon never sees
the discarded conflict. The existing `propertyItemList` path used by `@incr`
builds an `ImmutableMap` and rejects duplicate keys instead. Please reject a
repeated key here as well and add a negative parser test, so conflicting
snapshot/mode requests cannot be resolved implicitly.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2815,6 +2827,51 @@ public LogicalPlan visitTableName(TableNameContext ctx) {
return plan;
}
+ private TableScanParams visitTableOptionsHint(TableNameContext ctx) {
+ List<ParserRuleContext> hintContexts = Lists.newArrayList();
+ ParserRuleContext afterTableName =
+
tableHintMap.get(ctx.multipartIdentifier().getStop().getTokenIndex());
+ if (afterTableName != null) {
+ hintContexts.add(afterTableName);
+ }
+ ParserRuleContext afterRelation =
tableHintMap.get(ctx.getStop().getTokenIndex());
Review Comment:
[P1] Bind hints after relation modifiers
This misses valid placements where another relation component follows the
comment. For `FROM t@incr(...) /*+ OPTIONS('scan.snapshot-id'='1') */ AS f`,
`getTableHintMap` keys the hidden hint to the `@incr` closing parenthesis,
while this method probes only the table identifier and the final context token
(`f`). The OPTIONS hint is silently dropped, so even the new mutual-exclusion
check is bypassed. Time travel, partition/tablet, and similar intermediate
components have the same gap. Please associate hints with the containing
`TableNameContext` (or inspect every eligible token boundary) and add
modifier-plus-alias coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2815,6 +2827,51 @@ public LogicalPlan visitTableName(TableNameContext ctx) {
return plan;
}
+ private TableScanParams visitTableOptionsHint(TableNameContext ctx) {
+ List<ParserRuleContext> hintContexts = Lists.newArrayList();
+ ParserRuleContext afterTableName =
+
tableHintMap.get(ctx.multipartIdentifier().getStop().getTokenIndex());
+ if (afterTableName != null) {
+ hintContexts.add(afterTableName);
+ }
+ ParserRuleContext afterRelation =
tableHintMap.get(ctx.getStop().getTokenIndex());
+ if (afterRelation != null && afterRelation != afterTableName) {
+ hintContexts.add(afterRelation);
+ }
+
+ Map<String, String> options = null;
+ for (ParserRuleContext hintContext : hintContexts) {
+ SelectHintContext selectHintContext = (SelectHintContext)
hintContext;
+ for (HintStatementContext hintStatement :
selectHintContext.hintStatements) {
+ if (hintStatement.hintName() == null
+ ||
!"options".equalsIgnoreCase(hintStatement.hintName().getText())) {
+ continue;
+ }
+ if (options != null) {
+ throw new AnalysisException("Only one OPTIONS hint is
allowed for each table");
+ }
+ options = Maps.newLinkedHashMap();
+ for (HintAssignmentContext kv : hintStatement.parameters) {
+ if (kv.key == null || (kv.constantValue == null &&
kv.identifierValue == null)) {
+ throw new AnalysisException("OPTIONS hint requires
key-value assignments");
+ }
+ String key = visitIdentifierOrText(kv.key);
+ String value;
+ if (kv.constantValue != null) {
+ Literal literal = (Literal) visit(kv.constantValue);
+ value = literal.toLegacyLiteral().getStringValue();
Review Comment:
[P2] Preserve Boolean option text
Unquoted Booleans are corrupted here: `TRUE`/`FALSE` become legacy
`BoolLiteral`s whose `getStringValue()` is `"1"`/`"0"`. Paimon Boolean options
only accept `true` or `false`, so a valid hint such as
`OPTIONS('scan.plan-sort-partition'=TRUE)` fails during `Table.copy` instead of
enabling the option. Please use the existing `parseConstant` conversion (or
explicitly emit canonical Boolean text) and add coverage for unquoted Boolean
values.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,8 +924,14 @@ private Table getProcessedTable() throws UserException {
}
if (theScanParams != null && theScanParams.incrementalRead()) {
+ // System table handles are cached, so preserve query isolation by
applying dynamic
+ // options to a copied Paimon table instead of changing the shared
handle.
return baseTable.copy(getIncrReadParams());
}
+ if (theScanParams != null && theScanParams.isOptions()) {
+ // Apply per-query options to a copy so they cannot leak through
cached table handles.
+ return baseTable.copy(theScanParams.getMapParams());
Review Comment:
[P1] Bind the schema selected by snapshot options
`Table.copy` can change the row type here: in Paimon 1.3.1,
`scan.snapshot-id` resolves the target snapshot's `schemaId`, and data system
tables such as `audit_log` derive their row type from that copied table. Doris
has already cached the latest system-table columns and converted predicates
against the latest row type, though. After schema evolution, a query for an
older snapshot can therefore have latest-only projections silently filtered at
lines 610-614 and latest field indexes passed to a different historical schema.
Serializing this processed table (the subject of the existing thread) does not
fix that mismatch. Please resolve schema-changing options before
relation/predicate binding, or reject them for schema-dependent system tables,
and cover an evolved-schema snapshot.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2815,6 +2827,51 @@ public LogicalPlan visitTableName(TableNameContext ctx) {
return plan;
}
+ private TableScanParams visitTableOptionsHint(TableNameContext ctx) {
+ List<ParserRuleContext> hintContexts = Lists.newArrayList();
+ ParserRuleContext afterTableName =
+
tableHintMap.get(ctx.multipartIdentifier().getStop().getTokenIndex());
+ if (afterTableName != null) {
Review Comment:
[P1] Bind hints after relation modifiers
This misses valid placements where another relation component follows the
comment. For `FROM t@incr(...) /*+ OPTIONS('scan.snapshot-id'='1') */ AS f`,
`getTableHintMap` keys the hidden hint to the `@incr` closing parenthesis,
while this method probes only the table identifier and the final context token
(`f`). The OPTIONS hint is silently dropped, so even the new mutual-exclusion
check is bypassed. Time travel, partition/tablet, and similar intermediate
components have the same gap. Please associate hints with the containing
`TableNameContext` (or inspect every eligible token boundary) and add
modifier-plus-alias coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/NereidsParser.java:
##########
@@ -399,6 +411,23 @@ public static Map<Integer, ParserRuleContext>
getHintMap(String sql, CommonToken
return selectHintMap;
}
+ private static Map<Integer, ParserRuleContext> getTableHintMap(
+ CommonTokenStream tokenStream, Map<Integer, ParserRuleContext>
selectHintMap) {
+ Map<Integer, ParserRuleContext> tableHintMap = Maps.newHashMap();
+ Token previousDefaultToken = null;
+ for (Token token : tokenStream.getTokens()) {
+ ParserRuleContext hintContext =
selectHintMap.get(token.getStartIndex());
+ if (hintContext != null && previousDefaultToken != null) {
Review Comment:
[P2] Preserve all hints at the same table boundary
Adjacent hidden comments have no default-channel token between them, so both
are keyed to the same `previousDefaultToken` and the later `put` overwrites the
earlier context. Thus `t /*+ OPTIONS('scan.snapshot-id'='1') */ /*+
OPTIONS('scan.mode'='from-snapshot') */` silently loses the snapshot option
instead of triggering the intended one-OPTIONS-per-table error. Store all
contexts per anchor in source order (or reject the second one) and cover
separate adjacent comments.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2815,6 +2827,51 @@ public LogicalPlan visitTableName(TableNameContext ctx) {
return plan;
}
+ private TableScanParams visitTableOptionsHint(TableNameContext ctx) {
+ List<ParserRuleContext> hintContexts = Lists.newArrayList();
+ ParserRuleContext afterTableName =
+
tableHintMap.get(ctx.multipartIdentifier().getStop().getTokenIndex());
+ if (afterTableName != null) {
+ hintContexts.add(afterTableName);
+ }
+ ParserRuleContext afterRelation =
tableHintMap.get(ctx.getStop().getTokenIndex());
+ if (afterRelation != null && afterRelation != afterTableName) {
+ hintContexts.add(afterRelation);
+ }
+
+ Map<String, String> options = null;
+ for (ParserRuleContext hintContext : hintContexts) {
+ SelectHintContext selectHintContext = (SelectHintContext)
hintContext;
+ for (HintStatementContext hintStatement :
selectHintContext.hintStatements) {
+ if (hintStatement.hintName() == null
+ ||
!"options".equalsIgnoreCase(hintStatement.hintName().getText())) {
+ continue;
+ }
+ if (options != null) {
+ throw new AnalysisException("Only one OPTIONS hint is
allowed for each table");
+ }
+ options = Maps.newLinkedHashMap();
+ for (HintAssignmentContext kv : hintStatement.parameters) {
+ if (kv.key == null || (kv.constantValue == null &&
kv.identifierValue == null)) {
+ throw new AnalysisException("OPTIONS hint requires
key-value assignments");
+ }
+ String key = visitIdentifierOrText(kv.key);
+ String value;
+ if (kv.constantValue != null) {
+ Literal literal = (Literal) visit(kv.constantValue);
+ value = literal.toLegacyLiteral().getStringValue();
+ } else {
Review Comment:
[P2] Reject duplicate option keys
This unconditional `put` makes `/*+ OPTIONS('scan.snapshot-id'='1',
'scan.snapshot-id'='2') */` silently execute with snapshot 2; Paimon never sees
the discarded conflict. The existing `propertyItemList` path used by `@incr`
builds an `ImmutableMap` and rejects duplicate keys instead. Please reject a
repeated key here as well and add a negative parser test, so conflicting
snapshot/mode requests cannot be resolved implicitly.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2815,6 +2827,51 @@ public LogicalPlan visitTableName(TableNameContext ctx) {
return plan;
}
+ private TableScanParams visitTableOptionsHint(TableNameContext ctx) {
+ List<ParserRuleContext> hintContexts = Lists.newArrayList();
+ ParserRuleContext afterTableName =
+
tableHintMap.get(ctx.multipartIdentifier().getStop().getTokenIndex());
+ if (afterTableName != null) {
+ hintContexts.add(afterTableName);
+ }
+ ParserRuleContext afterRelation =
tableHintMap.get(ctx.getStop().getTokenIndex());
+ if (afterRelation != null && afterRelation != afterTableName) {
+ hintContexts.add(afterRelation);
+ }
+
+ Map<String, String> options = null;
+ for (ParserRuleContext hintContext : hintContexts) {
+ SelectHintContext selectHintContext = (SelectHintContext)
hintContext;
+ for (HintStatementContext hintStatement :
selectHintContext.hintStatements) {
+ if (hintStatement.hintName() == null
+ ||
!"options".equalsIgnoreCase(hintStatement.hintName().getText())) {
+ continue;
+ }
+ if (options != null) {
+ throw new AnalysisException("Only one OPTIONS hint is
allowed for each table");
+ }
+ options = Maps.newLinkedHashMap();
+ for (HintAssignmentContext kv : hintStatement.parameters) {
+ if (kv.key == null || (kv.constantValue == null &&
kv.identifierValue == null)) {
+ throw new AnalysisException("OPTIONS hint requires
key-value assignments");
+ }
+ String key = visitIdentifierOrText(kv.key);
+ String value;
Review Comment:
[P2] Preserve Boolean option text
Unquoted Booleans are corrupted here: `TRUE`/`FALSE` become legacy
`BoolLiteral`s whose `getStringValue()` is `"1"`/`"0"`. Paimon Boolean options
only accept `true` or `false`, so a valid hint such as
`OPTIONS('scan.plan-sort-partition'=TRUE)` fails during `Table.copy` instead of
enabling the option. Please use the existing `parseConstant` conversion (or
explicitly emit canonical Boolean text) and add coverage for unquoted Boolean
values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
