Gabriel39 commented on PR #66227:
URL: https://github.com/apache/doris/pull/66227#issuecomment-5707230762
Reviewed head `0d62a55072b5e094aca46eea05e5ffbfd0c64e28`. Thanks for
addressing the earlier findings. There are still the following issues and test
gaps to resolve:
1. **[P1] Do not strip lossy column casts before predicate pushdown.**
[`_resolve_field`](https://github.com/apache/doris/blob/0d62a55072b5e094aca46eea05e5ffbfd0c64e28/be/src/format/table/paimon_rust_predicate_converter.cpp#L378)
calls `expr_without_cast`. For a `DECIMAL(10,2)` column containing `1.24`,
`CAST(amount AS DECIMAL(10,1)) = 1.2` must retain the row, but this converter
builds equality against the original column and Rust rejects `1.24`. The Doris
residual cannot recover that row. The FE converter already rejects casted
operands, and its existing `rejectDecimalScaleCast` test establishes that this
cast survives planning. The pinned Rust predicate implementation accepts
decimal literals with different scales and compares their mathematical values,
so it does not reject this unsafe predicate as a type mismatch.
Please reject casted operands unless equivalence has been established.
Add converter tests and an actual Rust/JNI differential regression for decimal
scale reduction and CAST-related NULL semantics.
2. **[P2] Serialize the effective table options used by the query.**
[This
path](https://github.com/apache/doris/blob/0d62a55072b5e094aca46eea05e5ffbfd0c64e28/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java#L429)
reads `source.getPaimonTable()` and serializes its schema at line 457, whereas
JNI serializes the processed table. For reader-only relation options such as
`t@options('read.batch-size'='1')`, the snapshot path reuses the cached table
and `getProcessedTable()` subsequently applies the relation options.
Serializing the original table drops that override. The C API treats the
separately supplied options as FileIO properties only, while Rust obtains its
read batch size from the schema options.
Please serialize the same effective table used for planning/JNI, and add
tests for relation-option precedence and the options actually received by the
Rust reader.
3. **[P2] Fix the Spark DDL in the new regression.**
[`test_paimon_rust_reader_eq_for_null.groovy`](https://github.com/apache/doris/blob/0d62a55072b5e094aca46eea05e5ffbfd0c64e28/regression-test/suites/external_table_p0/paimon/test_paimon_rust_reader_eq_for_null.groovy#L56)
sends `TIMESTAMP(6)` to Spark at lines 56 and 65. Spark's [datatype
parser](https://github.com/apache/spark/blob/v3.5.1/sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala#L54-L86)
accepts TIMESTAMP without precision parameters, so this setup is rejected
before the assertions. Please use supported Spark syntax and explicitly
establish NTZ/LTZ semantics; simply removing `(6)` does not establish the NTZ
semantics claimed by the comments.
4. **[P2] Ensure the specialized regression actually exercises Rust.**
[The two comparison
legs](https://github.com/apache/doris/blob/0d62a55072b5e094aca46eea05e5ffbfd0c64e28/regression-test/suites/external_table_p0/paimon/test_paimon_rust_reader_eq_for_null.groovy#L115)
only toggle `enable_paimon_rust_reader`. These append tables can be converted
into raw native splits, which `getSplits()` prioritizes when
`force_jni_scanner` is false. Both legs can therefore use the native reader,
bypassing the new converter and materializer.
Please set `force_jni_scanner=true` for the differential comparisons,
restore the setting afterward, and verify the actual reader path through the
profile or equivalent instrumentation. For the fractional-timestamp join, also
force/verify an IN runtime filter arriving before split opening; an
unconstrained join does not guarantee that path. The separate V2 smoke suite
forces logical splits but does not cover these specialized cases. Please run
the corrected external suite with Paimon tests enabled and provide the results.
**Directory placement:** Please move `paimon_rust_predicate_converter.cpp`
and `.h` from `be/src/format/table/` to `be/src/format_v2/table/`, and move the
corresponding test from `be/test/format/table/` to `be/test/format_v2/table/`.
Update the includes and any build references accordingly. This converter is
consumed by the V2 Rust reader and relies on V2's rewritten table-global slot
references; its implementation and tests should live alongside that V2 path.
This follow-up is based on static call-chain and dependency-source review. I
have not run this PR's build or end-to-end tests.
--
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]
