github-actions[bot] commented on code in PR #63850:
URL: https://github.com/apache/doris/pull/63850#discussion_r3400157533
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -447,6 +476,83 @@ private Optional<LogicalPlan> handleMetaTable(TableIf
table, UnboundRelation unb
return Optional.of(new
LogicalTVFRelation(unboundRelation.getRelationId(), tvf, ImmutableList.of()));
}
+ private StreamScanType checkChangeScanCondition(OlapTable olapTable,
TableScanParams scanParams)
+ throws AnalysisException {
+ if (!olapTable.needRowBinlog()) {
+ throw new AnalysisException("INCR query requires ROW binlog
enabled on base table "
+ +
"(PROPERTIES('binlog.enable'='true','binlog.format'='ROW')). "
Review Comment:
This still treats an omitted `incrementType` as `MIN_DELTA` during analysis,
even though the scan node defaults an omitted key to `DETAIL`
(`parseBinlogScanType()` initializes `DETAIL` and only changes when the key is
present), and the new regression tests assert `@incr()` equals explicit
`DETAIL` for the all-history case. That mismatch makes valid default detail
reads fail before planning; for example, a UNIQUE MOW table with row binlog
enabled but `binlog.need_historical_value=false` supports `@incr(...,
"incrementType"="DETAIL")`, but `@incr('startTimestamp'=...)` hits this line,
becomes MIN_DELTA, and throws the historical-value error. This is distinct from
the earlier DEFAULT enum cleanup: the key is now absent, but FE validation and
BE execution still choose different defaults. Please canonicalize the default
once, or default this validator to DETAIL, before enforcing MIN_DELTA-only
prerequisites.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -235,6 +255,9 @@ private LogicalPlan makeOlapScan(TableIf table,
UnboundRelation unboundRelation,
throw new AnalysisException("Table " + olapTable.getName()
+ " doesn't have materialized view " +
indexName.get());
}
+ if (isChangeRead && olapTable.getBaseIndexId() != indexId) {
+ throw new AnalysisException("Change read is not supported
on non-base index " + indexName.get());
+ }
Review Comment:
This change-read branch needs to run before the `PREAGGOPEN` early return
above. When `isChangeRead` is true, the table has already been replaced with
`RowBinlogTableWrapper`, but the early return skips `checkChangeScanCondition`,
`withTableScanParams`, and `checkAndAddChangeScanFilter`. A query such as
`SELECT /*+PREAGGOPEN*/ ... FROM t@incr('startTimestamp'=...,
'incrementType'='MIN_DELTA')` therefore reaches `OlapScanNode` with `scanParams
== null`: no start/end TSO filter is installed and `parseBinlogScanType()`
returns `NONE`, so BE does not execute the requested MIN_DELTA/DETAIL
change-reader path. Please apply the change-read setup before honoring the
hint, or reject `PREAGGOPEN` for `@incr` scans.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOlapScan.java:
##########
@@ -631,7 +678,7 @@ public LogicalOlapScan
appendVirtualColumns(List<NamedExpression> additionalVirt
selectedIndexId, indexSelected, preAggStatus,
manuallySpecifiedPartitions,
hints, cacheSlotWithSlotName, cachedOutput, tableSample,
directMvScan, colToSubPathsMap,
Review Comment:
The BEFORE-column filtering only runs when `incrementType` is present. For
the parser-supported default form (`tbl@incr()` or timestamp-only `@incr`),
`scanParams` is present but the map lacks `OLAP_INCREMENT_TYPE`, so
`skipBinlogBeforeColumn` is false. On MOW tables with
`binlog.need_historical_value=true`, `getBaseSchema(true)` contains visible
`__BEFORE__...` row-binlog columns, and `SELECT * FROM t@incr()` will expose
them even though `SELECT * FROM t@incr("incrementType"="DETAIL")` hides them.
The tests currently compare selected columns only, so this output-schema
regression is missed. Please base this on `scanParams.incrementalRead()` or
canonicalize the default increment type so default detail and explicit detail
have the same schema.
--
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]
