morningman commented on PR #68825: URL: https://github.com/apache/doris/pull/68825#issuecomment-6162507196
I compared this PR at `13e491640aa` with branch-4.1 #66227 while working on bringing branch-4.x changes back to master, mostly by reading the code. The CI results below are from this head; when I wrote this, BE UT had passed, External had two failures and was still running, and FE UT and NonConcurrent were still running. The BE reader, the predicate converter and the BE unit tests match branch-4.1 closely, and every FE gate of `PaimonScanNode` has a counterpart in `PaimonRustReaderSelector` / `PaimonRustReaderCapabilities`. 13e491640aa also fixed the sliced Arrow batches: the row range is now read from the original batch. The points below are where it still differs. **1. The Performance build does not compile.** GCC stops at `paimon_rust_table_reader.cpp:142` with `-Werror=subobject-linkage`: the members of `PaimonRustTableReader::PaimonHandles` use `table_ptr`, `read_builder_ptr` and the other aliases from the anonymous namespace. GCC does not report this for the main source file, but master compiles `Format` as a unity build, so the file is compiled inside `unity_7_cxx.cxx`. Performance is the only GCC build; it failed the same way at 19a6bab262e. Moving the deleters and aliases into a named namespace should fix it. **2. With the Rust reader on, COUNT(col), MIN and MAX on append tables no longer use footer aggregates (13e491640aa).** Compatible splits now go to the Rust reader before the native check (`PaimonScanPlanProvider` L991-1002). branch-4.1 keeps raw-file splits on the native reader on purpose: "only the native file reader can use footer aggregates for COUNT(col) and MIN/MAX" (`PaimonScanNode` L1238-1241). Its `test_paimon_rust_reader_compatibility` runs the footer-aggregate queries with the Rust reader both on and off and asserts the native reader (L113-138). Here that block now runs only with the Rust reader off (L119-123), and the forced case that checked that the Rust reader returns real rows to the upper aggregate expects JNI (L147-148), so neither side is covered any more. Either keep raw-file splits ahead of the Rust reader as branch-4.1 does, or at least do so when the scan pushes down an aggregate; in both cases a Rust-enabled run of that block would catch a regression. Also, Rust ranges are now planned before the `ignore_split_type` checks, so `IGNORE_JNI` no longer drops them as it does on branch-4.1. **3. `test_paimon_rust_reader_eq_for_null` fails because the DDL mapping is not ported.** It fails at L222-223: expected `[[1, abc, 1.23], [2, xyz, 2.35]]`, but got `[[1, abcdef, 1.23], [2, xyzuvw, 2.35]]`. The Javadoc of `PaimonTypeMapping.toPaimonType` says CHAR/VARCHAR bounds and timestamp precision are preserved, but the code still maps CHAR(n) and VARCHAR(n) to VARCHAR(2147483647) and DATETIME to TIMESTAMP(6), and `PaimonTypeMappingToPaimonTest` still asserts that. So `MODIFY COLUMN v VARCHAR(3)` (L208) widens the column and Doris reads it back as STRING, which is also why `truncate_char_or_varchar_columns` (L220) does not help. I opened #69045 with the branch-4.1 mapping and its tests. Its Javadoc is the same text as yours, so the two PRs do not conflict. (The profile changes of #66227 are in #69044.) **4. `test_paimon_rust_reader_compatibility` fails on the new routing.** The check before the ALTER at L156 expects no Rust reader, but `string_to_temporal_0` is an append table, which now goes to the Rust reader; branch-4.1 expects the Rust reader there too (L149). The suite stops at this failure, so the checks after it have not run. They may fail next: their baseline is now the native reader (`force_jni_scanner=false` at L47; branch-4.1 sets it to true, so its baseline is JNI), while the expected values are Java cast results (`1970-02-12`, `1970-01-01 00:00:00.042`, epoch seconds, and `127` for BIGINT to TINYINT). **5. The session variable is never fuzzed.** `enable_paimon_rust_reader` has `fuzzy = true`, but `setFuzzyForCatalog` does not randomize it; branch-4.1 sets `this.enablePaimonRustReader = random.nextBoolean()` there. On master the annotation only affects printing, so CI never runs the Paimon suites with the Rust reader. **6. The selector still rejects some scans that branch-4.1 reads with the Rust reader.** These only cost performance, but they would be worth stating in the description: - `deletion-vectors.merge-on-read=true` sends every table to JNI. branch-4.1 only does that for partial-update and aggregation tables with deletion vectors, and its tests expect deduplicate + merge-on-read to use the Rust reader. - The COUNT(*) range is always JNI. - Any access path deeper than one level falls back. - A TIMESTAMP(7..9) column anywhere in the current schema sends the whole table to JNI, even when it is not projected. **7. FE tests.** branch-4.1 adds 73 FE test methods, and about 50 have no counterpart here. In particular, nothing covers the 45 schema-evolution, aggregation and primary-key cases of `PaimonRustReaderCapabilities`. The class is nearly identical to branch-4.1, so `PaimonRustReaderCompatibilityTest`, `PaimonRustReaderEvolutionTest`, `PaimonRustReaderLegacySchemaTest` and the capability cases of `PaimonScanNodeTest` should port with small changes. The Rust range payload (schema JSON without time-travel selectors, relation options, branch) has no FE test either. One change goes the other way: the `EQ_FOR_NULL` change in `paimon_rust_predicate_converter.cpp` is a real fix. branch-4.1 pushes `IS NULL` for `col <=> non-null literal` and drops matching rows when `NULL_SAFE_EQUAL_TO_EQUAL` is disabled. `EqForNullLiteralIsPushed` only checks that some predicate is pushed, so it also passes with the branch-4.1 code; a test that reads rows would pin the fix. -- 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]
