github-actions[bot] commented on code in PR #65464:
URL: https://github.com/apache/doris/pull/65464#discussion_r3592439276
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalMaxComputeTableSink.java:
##########
@@ -141,15 +141,16 @@ public PhysicalProperties getRequirePhysicalProperties() {
DistributionSpecHiveTableSinkHashPartitioned shuffleInfo
= new DistributionSpecHiveTableSinkHashPartitioned();
shuffleInfo.setOutputColExprIds(exprIds);
- // Require local sort by partition columns so that rows for the
same partition
- // are grouped together. MaxCompute Storage API streams dynamic
partition data
- // and will close a partition writer once it sees a different
partition;
- // unsorted data causes "writer has been closed" errors.
+ // Sort after the partition shuffle so rows for the same partition
are grouped
+ // in each final writer stream. MaxCompute Storage API closes a
partition writer
+ // once it sees another partition, so unsorted data causes "writer
has been closed".
+ // A regular OrderSpec is intentional: MustLocalSortOrderSpec
would sort before
+ // the shuffle and add an unnecessary merging exchange afterwards.
List<OrderKey> orderKeys = columnIdx.stream()
.map(idx -> new OrderKey(child().getOutput().get(idx),
true, false))
.collect(Collectors.toList());
return new PhysicalProperties(shuffleInfo)
- .withOrderSpec(new MustLocalSortOrderSpec(orderKeys));
+ .withOrderSpec(new OrderSpec(orderKeys));
Review Comment:
[P1] Keep MaxCompute ordering independent of strict-consistency DML
This property is never requested when `enable_strict_consistency_dml=false`,
and it is always skipped in cloud mode because
`SessionVariable.isEnableStrictConsistencyDml()` returns false there. A dynamic
insert can therefore still reach `VMCTableWriter` with neither the partition
exchange nor local sort, despite the writer requiring each partition to be
contiguous; an A/B/A input can still fail with `writer has been closed`. The
switch controls replica consistency, not connector-writer correctness, so
MaxCompute should request its dynamic-partition properties regardless (and
preserve them through shuffle pruning), with strict-disabled/cloud-aware
planning coverage.
##########
regression-test/suites/external_table_p2/maxcompute/write/test_mc_write_static_partitions.groovy:
##########
@@ -213,10 +223,10 @@ suite("test_mc_write_static_partitions", "p2,external") {
region STRING
) PARTITION BY (ds, region)()
"""
- // Explain: partial static partition (ds static, region dynamic)
should HAVE SORT node
+ // A remaining dynamic partition column also requires shuffle followed
by local sort.
explain {
sql("INSERT INTO ${tb7} PARTITION(ds='20250101') VALUES (1, 'v1',
'bj')")
- contains "SORT"
+ check { explainString ->
checkPartitionShuffleThenSort(explainString) }
Review Comment:
[P1] Execute the partial-static path before treating it as covered
This EXPLAIN assertion passes, but the INSERT itself is never run. Today any
nonempty static spec selects `builder.partition(...)` in `MCTransaction` and
`_has_static_partition` in `VMCTableWriter`; BE then removes all table
partition columns (`ds` and `region`) and writes to the single `ds=...` writer.
The remaining dynamic `region` value is therefore dropped instead of being
routed dynamically. Please either reject partial-static MaxCompute writes or
implement the mixed static/dynamic session and writer path, then execute this
INSERT and query the target partition so the regression proves runtime behavior
rather than only planner shape.
--
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]
