JingsongLi commented on code in PR #7886:
URL: https://github.com/apache/paimon/pull/7886#discussion_r3458861961
##########
paimon-core/src/main/java/org/apache/paimon/append/AppendOnlyWriter.java:
##########
@@ -159,11 +164,18 @@ public AppendOnlyWriter(
this.statsCollectorFactories = statsCollectorFactories;
this.maxDiskSize = maxDiskSize;
this.fileIndexOptions = fileIndexOptions;
+ this.coreOptions = coreOptions;
- this.sinkWriter =
- useWriteBuffer
- ? createBufferedSinkWriter(spillable)
- : new DirectSinkWriter<>(this::createRollingRowWriter);
+ // Determine if we need to enable sorting based on clustering
configuration
+ List<String> clusteringColumns = coreOptions.clusteringColumns();
+ this.sortEnabled =
+ coreOptions.clusteringIncrementalEnabled()
+ && coreOptions.clusteringIncrementalOptimizeWrite()
+ && coreOptions.clusteringIncrementalMode()
+ ==
CoreOptions.ClusteringIncrementalMode.LOCAL_SORT
+ && !clusteringColumns.isEmpty();
+
+ this.sinkWriter = createSinkWriter(useWriteBuffer, spillable);
Review Comment:
This makes local sort silently depend on `write-buffer-for-append`, whose
default is `false`. Even when all clustering-local-sort options above make
`sortEnabled=true`, the default append path passes `useWriteBuffer=false`, so
`createSinkWriter` falls through to `DirectSinkWriter` and no sorting happens.
The new test sets `write-buffer-for-append=true`, but the incremental
clustering configuration/docs do not require users to set that extra option, so
the feature will appear enabled while being bypassed in the common path. Could
local-sort force the buffered writer here (or fail fast/document and test the
required `write-buffer-for-append=true` setting)?
--
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]
