github-actions[bot] commented on code in PR #66187:
URL: https://github.com/apache/doris/pull/66187#discussion_r3670590359
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -1508,6 +1508,42 @@ public Partition getPartition(String partitionName,
boolean isTempPartition) {
}
}
+ @Override
+ public Partition getMaxVisiblePartition() {
+ PartitionInfo partitionInfo = getPartitionInfo();
+ PartitionType type = partitionInfo.getType();
+ // LIST has no natural "latest"; UNPARTITIONED has a single partition.
+ // Neither participates in the range-max scan below.
+ if (type == PartitionType.LIST) {
+ // no meaningful "max" partition -> caller builds an empty query
+ return null;
+ }
+ if (type == PartitionType.UNPARTITIONED) {
+ for (Partition partition : nameToPartition.values()) {
+ if (partition.getVisibleVersion() >
Partition.PARTITION_INIT_VERSION) {
+ return partition;
+ }
+ }
+ return null;
+ }
+ Partition result = null;
+ PartitionKey maxUpperBound = null;
+ for (Partition partition : nameToPartition.values()) {
+ if (partition.getVisibleVersion() <=
Partition.PARTITION_INIT_VERSION) {
Review Comment:
[P1] Batch cloud version lookups before scanning partitions
On a cloud table, every iteration here dispatches through
`CloudPartition.getVisibleVersion()`. Once the cache entry has expired, that is
a synchronous `batchMode=false` meta-service RPC. Binding runs after
`StatementContext.lock()` and the planner releases that table read lock only
after planning, so a table with N expired partitions now incurs N sequential
network round trips while holding metadata locks. The existing
`Partition.getVisibleVersions()` / `CloudPartition.getSnapshotVisibleVersion()`
path already fetches an aligned version vector in one batch. Please use that
batch result to choose the greatest upper bound; otherwise the feature intended
to reduce planning overhead can instead scale with N RPC latencies and combine
observations from different instants.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -1508,6 +1508,42 @@ public Partition getPartition(String partitionName,
boolean isTempPartition) {
}
}
+ @Override
+ public Partition getMaxVisiblePartition() {
+ PartitionInfo partitionInfo = getPartitionInfo();
+ PartitionType type = partitionInfo.getType();
+ // LIST has no natural "latest"; UNPARTITIONED has a single partition.
+ // Neither participates in the range-max scan below.
+ if (type == PartitionType.LIST) {
+ // no meaningful "max" partition -> caller builds an empty query
+ return null;
+ }
+ if (type == PartitionType.UNPARTITIONED) {
+ for (Partition partition : nameToPartition.values()) {
+ if (partition.getVisibleVersion() >
Partition.PARTITION_INIT_VERSION) {
Review Comment:
[P1] Include current-transaction partitions in the max selection
This test only sees committed `visibleVersion`, but Doris's transaction-load
path intentionally treats partitions containing earlier subtransaction writes
as readable: `PruneEmptyPartition.selectNonEmptyPartitionIdsForTxnLoad()` adds
them through `TransactionEntry.getPartitionSubTxnIds()`. By selecting one
committed partition here—or returning null before that rewrite runs—the new
path removes the higher pending partition from the candidate set. For example,
after one insert-select subtransaction writes `p2`, a later `INSERT INTO dst
SELECT ... FROM src max_visible_partition()` in the same transaction scans
older `p1` (or an empty relation) instead of `p2`. Please merge
current-transaction partition evidence into the max ordering before deciding
the result is empty.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -260,6 +261,15 @@ private LogicalPlan makeOlapScan(TableIf table,
UnboundRelation unboundRelation,
} else if (unboundRelation.getScanParams() != null) {
unboundRelation.getScanParams().validateOlapTable();
}
+ // max_visible_partition() with no visible partition -> empty result
instead of
+ // falling through to a full-table scan (empty partIds otherwise means
"all partitions")
+ if (unboundRelation.isMaxVisiblePartition() &&
CollectionUtils.isEmpty(partIds)) {
Review Comment:
[P1] Validate and shape all scan modifiers before returning empty
This return happens before direct-index lookup/compatibility, manual-tablet
marking, `PREAGGOPEN`/`@incr` compatibility, and `validateTimeTravel()`.
Consequently `empty_t INDEX missing max_visible_partition()` succeeds instead
of reporting the missing index, and unsupported or malformed time travel is
accepted only while the table is empty. For a valid non-base index, this
default scan also supplies the base-table output rather than
`getOutputByIndex()`; for `@incr` it drops the scan params used to shape the
row-binlog output. Please resolve and validate the complete requested scan
first, then derive the empty relation from that resolved output so analysis and
metadata do not depend on whether data currently exists.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -1014,14 +1033,19 @@ private Plan parseAndAnalyzeView(TableIf view, String
ddlSql, CascadesContext pa
private List<Long> getPartitionIds(TableIf t, UnboundRelation
unboundRelation, List<String> qualifier) {
List<String> parts = unboundRelation.getPartNames();
- if (CollectionUtils.isEmpty(parts)) {
+ if (CollectionUtils.isEmpty(parts) &&
!unboundRelation.isMaxVisiblePartition()) {
return ImmutableList.of();
}
if (!t.isManagedTable()) {
throw new AnalysisException(String.format(
"Only OLAP table is support select by partition for now,"
+ "Table: %s is not OLAP table", t.getName()));
}
+ if (unboundRelation.isMaxVisiblePartition()) {
+ Partition partition = t.getMaxVisiblePartition();
Review Comment:
[P1] Resolve the maximum partition at the requested snapshot
The grammar permits `FOR VERSION/TIME AS OF` immediately before this clause,
but this call uses today's partition versions and only later does
`buildTimeTravelPlan()` apply `commit_tso <= targetTso` to the already
restricted scan. With `p1` first populated at TSO 100 and higher `p2` first
populated at TSO 200, `FROM t FOR VERSION AS OF 150 max_visible_partition()`
becomes:
`Filter(commit_tso <= 150) -> LogicalOlapScan(partitions=[p2])`
and returns empty instead of the rows from `p1`; the MOW union receives the
same wrong `partIds`. Please either reject this modifier combination or
determine non-emptiness/max ordering against the requested snapshot.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundRelation.java:
##########
@@ -53,6 +53,7 @@ public class UnboundRelation extends LogicalRelation
implements Unbound, BlockFu
private final List<String> partNames;
private final List<Long> tabletIds;
private final boolean isTempPart;
+ private final boolean isMaxVisiblePartition;
Review Comment:
[P2] Include this scan modifier in the SQL digest
The new flag is preserved by the copy/equality paths, but
`UnboundRelation.toDigest()` never emits it. `LogicalPlanAdapter` captures that
digest and `AuditLogHelper` hashes it into `SqlDigest`, so `SELECT ... FROM t`
and `SELECT ... FROM t max_visible_partition()` are grouped as the same
statement despite radically different scan scope and performance. Other
semantic scan modifiers such as `INDEX`, `TABLET`, scan params, and snapshots
are represented there. Please append a stable `MAX_VISIBLE_PARTITION()` token
when this flag is set and add a digest assertion.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -1508,6 +1508,42 @@ public Partition getPartition(String partitionName,
boolean isTempPartition) {
}
}
+ @Override
+ public Partition getMaxVisiblePartition() {
+ PartitionInfo partitionInfo = getPartitionInfo();
+ PartitionType type = partitionInfo.getType();
+ // LIST has no natural "latest"; UNPARTITIONED has a single partition.
+ // Neither participates in the range-max scan below.
+ if (type == PartitionType.LIST) {
+ // no meaningful "max" partition -> caller builds an empty query
+ return null;
+ }
+ if (type == PartitionType.UNPARTITIONED) {
+ for (Partition partition : nameToPartition.values()) {
+ if (partition.getVisibleVersion() >
Partition.PARTITION_INIT_VERSION) {
+ return partition;
+ }
+ }
+ return null;
+ }
+ Partition result = null;
+ PartitionKey maxUpperBound = null;
+ for (Partition partition : nameToPartition.values()) {
+ if (partition.getVisibleVersion() <=
Partition.PARTITION_INIT_VERSION) {
+ continue;
+ }
+ // RANGE: pick the partition with the greatest range upper bound,
so it works for
+ // date/number partitions regardless of the partition name width
+ PartitionKey upper = ((RangePartitionItem)
partitionInfo.getItem(partition.getId()))
+ .getItems().upperEndpoint();
+ if (maxUpperBound == null || upper.compareTo(maxUpperBound) > 0) {
+ maxUpperBound = upper;
+ result = partition;
Review Comment:
[P1] Re-resolve the max partition before reusing a prepared point plan
The chosen ID is baked into `LogicalOlapScan`, and a qualifying point query
can then be cached as a server-prepared short-circuit `OlapScanNode`.
`ExecuteCommand` reuses that node when `ShortCircuitQueryContext.isReusable()`
succeeds, but that check covers only dropped state, schema version, table name,
and a session limit—not visible/table/partition versions. Prepare and execute
while `p1` is the maximum, commit data into higher `p2` without changing the
schema, then execute for a key in `p2`: the cached node remains pinned to `p1`
and returns the wrong result. Please disable short-circuit reuse for this
data-dependent modifier or include the selection/data version and force
rebinding when it changes.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -751,6 +761,15 @@ private LogicalPlan getLogicalPlan(TableIf table,
UnboundRelation unboundRelatio
}
List<String> qualifierWithoutTableName = qualifiedTableName.subList(0,
qualifiedTableName.size() - 1);
+ // max_visible_partition() only works on OLAP-backed tables; reject
others explicitly
+ // so the flag is not silently ignored.
+ if (unboundRelation.isMaxVisiblePartition()) {
Review Comment:
[P1] Enforce this restriction before every relation early return
This guard is not reached for two parser-accepted targets. `WITH c AS (...)
SELECT * FROM c max_visible_partition()` returns a
`LogicalCTEConsumer`/recursive work-table reference after
`rejectScanParamsOnCte()` (which does not inspect this flag), so it scans all
CTE rows. Likewise a resolved external `$` system table returns from
`handleMetaTable()` above this check as a file/TVF relation. Both silently
discard semantics that ordinary external tables reject. Please invoke a shared
modifier validation before those early returns, or explicitly reject the flag
in each path, and add CTE plus native/TVF meta-table negatives.
--
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]
