github-actions[bot] commented on code in PR #65912:
URL: https://github.com/apache/doris/pull/65912#discussion_r3635011590
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -588,9 +596,35 @@ private static String[] splitParam(String value, String
delimiter) {
private Map<String, String> buildTableOptions(Map<String, String>
tableOptions) {
Map<String, String> options = new HashMap<>(tableOptions);
options.put(CoreOptions.READ_BATCH_SIZE.key(),
String.valueOf(batchSize));
+ String localMillis = params.get(PAIMON_OPTION_PREFIX +
DORIS_FILE_CREATION_TIME_LOCAL_MILLIS);
+ if (localMillis != null) {
+ long lowerBound = Long.parseLong(localMillis);
+ long epochMillis = toFileCreationTimeEpochMillis(lowerBound,
ZoneId.systemDefault());
+ String existingMillis = params.get(
+ PAIMON_OPTION_PREFIX +
DORIS_FILE_CREATION_TIME_EXISTING_MILLIS);
+ if (existingMillis != null) {
+ // The query predicate is an additional restriction and must
never weaken a
+ // stronger cutoff already configured on the wrapped
FileStoreTable.
+ epochMillis = Math.max(epochMillis,
Long.parseLong(existingMillis));
+ }
+ options.put(CoreOptions.SCAN_FILE_CREATION_TIME_MILLIS.key(),
String.valueOf(epochMillis));
Review Comment:
**P1: Skip this pushdown for incompatible Paimon startup modes.** A wrapped
table with a valid setting such as `scan.mode=latest-full` now fails to open:
`FilesTable.copy` delegates to `FileStoreTable.copyInternal`, this option is
merged into the existing schema, and Paimon 1.3.1
`SchemaValidation.validateStartupMode` requires
`scan.file-creation-time-millis` to be absent for `latest-full`, snapshot,
timestamp, and incremental modes. Before this change the same `$files` query
ran and relied on the retained Doris predicate; now JNI initialization throws.
Please inspect the exact wrapped table's startup configuration and omit the
optimization unless it is compatible with `FROM_FILE_CREATION_TIME`, with real
`FilesTable` tests for conflicting modes. This is distinct from the existing
stronger-cutoff thread: validation fails before the two cutoff values can be
compared.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -928,4 +968,76 @@ private Table getProcessedTable() throws UserException {
}
return baseTable;
}
+
+ @VisibleForTesting
+ static OptionalLong extractFileCreationTimeLowerBound(List<Expr>
conjuncts) {
+ OptionalLong result = OptionalLong.empty();
+ for (Expr conjunct : conjuncts) {
+ result = maxLowerBound(result,
extractFileCreationTimeLowerBound(conjunct));
+ }
+ return result;
+ }
+
+ private static OptionalLong extractFileCreationTimeLowerBound(Expr expr) {
+ if (expr instanceof CompoundPredicate) {
+ CompoundPredicate predicate = (CompoundPredicate) expr;
+ if (predicate.getOp() != CompoundPredicate.Operator.AND) {
+ return OptionalLong.empty();
+ }
+ return
maxLowerBound(extractFileCreationTimeLowerBound(predicate.getChild(0)),
+ extractFileCreationTimeLowerBound(predicate.getChild(1)));
+ }
+ if (!(expr instanceof BinaryPredicate)) {
+ return OptionalLong.empty();
+ }
+
+ BinaryPredicate predicate = (BinaryPredicate) expr;
+ BinaryPredicate.Operator operator = predicate.getOp();
+ SlotRef slot = convertToDirectSlotRef(predicate.getChild(0));
+ LiteralExpr literal = convertToLiteral(predicate.getChild(1));
+ if (slot == null || literal == null) {
+ slot = convertToDirectSlotRef(predicate.getChild(1));
+ literal = convertToLiteral(predicate.getChild(0));
+ operator = operator.commutative();
+ }
+ if (slot == null || literal == null
+ ||
!PAIMON_FILE_CREATION_TIME_COLUMN.equalsIgnoreCase(slot.getColumnName())
+ || (operator != BinaryPredicate.Operator.GE && operator !=
BinaryPredicate.Operator.GT)) {
+ return OptionalLong.empty();
+ }
+
+ Object value = new TimestampType(3).accept(new
PaimonValueConverter(literal));
Review Comment:
**P1: Keep pre-epoch cutoffs from moving forward.** This reuses
`PaimonValueConverter.visit(TimestampType)`, which leaves
`Calendar.MILLISECOND` populated and computes `getTimeInMillis() / 1000 * 1000
+ micros / 1000`; Java division truncates negative values toward zero. For
`creation_time >= '1969-12-31 23:59:59.500'`, the intended zone-free cutoff is
`-500`, but the converter can emit `+500`. On a UTC BE, Paimon's manifest
filter then drops a supported file timestamp such as epoch `100`, although the
retained Doris predicate accepts it. Please derive this cutoff with proleptic
local-time arithmetic such as Paimon's `Timestamp.fromLocalDateTime(...)` (or
equivalent `java.time` conversion) and add a pre-epoch boundary test. This is
separate from the timezone/DST threads because it reproduces in UTC before zone
conversion.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -588,9 +596,35 @@ private static String[] splitParam(String value, String
delimiter) {
private Map<String, String> buildTableOptions(Map<String, String>
tableOptions) {
Map<String, String> options = new HashMap<>(tableOptions);
options.put(CoreOptions.READ_BATCH_SIZE.key(),
String.valueOf(batchSize));
+ String localMillis = params.get(PAIMON_OPTION_PREFIX +
DORIS_FILE_CREATION_TIME_LOCAL_MILLIS);
+ if (localMillis != null) {
+ long lowerBound = Long.parseLong(localMillis);
+ long epochMillis = toFileCreationTimeEpochMillis(lowerBound,
ZoneId.systemDefault());
+ String existingMillis = params.get(
+ PAIMON_OPTION_PREFIX +
DORIS_FILE_CREATION_TIME_EXISTING_MILLIS);
+ if (existingMillis != null) {
+ // The query predicate is an additional restriction and must
never weaken a
+ // stronger cutoff already configured on the wrapped
FileStoreTable.
+ epochMillis = Math.max(epochMillis,
Long.parseLong(existingMillis));
+ }
+ options.put(CoreOptions.SCAN_FILE_CREATION_TIME_MILLIS.key(),
String.valueOf(epochMillis));
Review Comment:
**P1: Preserve fallback-branch partition ownership.** `FilesTable.copy`
propagates this cutoff through `FallbackReadFileStoreTable.copy` into both the
main and fallback children. When `FilesRead` later expands the split,
`FallbackReadScan.plan()` marks a partition as main-owned only if the cutoff
left a surviving main split. For example, with a main file at time `100`, a
fallback file for the same partition at `1000`, and `creation_time >= 500`, the
old scan selects the main partition and the retained predicate returns no row;
this pushdown first removes the main split, activates the fallback partition,
and returns its `1000` row. Please skip this optimization for fallback-read
tables (or fix ownership before applying the cutoff) and add a real
fallback-backed `$files` test. This uses compatible startup options and one
exact table revision, so it is separate from the validation and cache-drift
findings.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -918,6 +947,17 @@ private Table getProcessedTable() throws UserException {
if (getQueryTableSnapshot() != null) {
throw new UserException("Paimon system tables do not support
time travel.");
}
+ PaimonSysExternalTable systemTable = (PaimonSysExternalTable)
source.getExternalTable();
+ if
(PAIMON_FILES_SYSTEM_TABLE_TYPE.equalsIgnoreCase(systemTable.getSysTableType()))
{
+ fileCreationTimeLowerBound =
extractFileCreationTimeLowerBound(conjuncts);
+ String existingLowerBound = systemTable.getTableProperties()
Review Comment:
**P1: Read the existing cutoff from the same table revision that is
serialized.** The `$files` wrapper comes from a direct
`PaimonExternalCatalog.getPaimonTable(...)` load, while this value comes
through the source table's independent `PaimonExternalMetaCache` handle. After
metadata drift those handles can disagree. For example, a cached cutoff of
`1000` plus a newly loaded wrapper cutoff of `2000` and query cutoff `500`
sends `1000`; the BE copy then overwrites the wrapper's actual `2000`,
admitting files in `[1000,2000)` that its scan restriction excluded. Reverse
drift can over-prune. Please derive the existing value from the exact wrapped
table/revision (or skip the optimization when identity is not guaranteed) and
test deliberately divergent handles. This is distinct from the prior
stronger-cutoff thread: the new preservation value itself can be stale.
--
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]
