Repository navigation
[python] Align DATE partition paths with Java - #10319
zhengguangzhuo wants to merge 7 commits into
Conversation
|
Thanks for working on this issue. I reported it and also attempted a fix in #10045, which I later closed because of compatibility concerns. I think this implementation still misses some of the difficulties I encountered. There is a read-after-write regression with composite partitions. With The historical-directory test also writes with the new code and then renames the entire directory. That doesn’t cover historical and new paths coexisting in the same logical partition/bucket, or compatibility between client versions. We need a compatibility matrix covering Java and Python writers/readers, old and new versions, and both partition naming modes, with the full Cartesian product of supported combinations. This should include different writers appending to the same partition/bucket, verifying that readers return all expected rows, and checking reads after rollback. I previously tried adding compatibility handling for different path formats, but the additional lookups caused performance regressions. We haven’t settled on how to handle that tradeoff, so this change needs performance evaluation alongside the compatibility tests. Could you address these gaps before we proceed with the write-path change? Preserving reads of historical ISO directories alone isn’t enough to establish compatibility. Please also use English for the added comments and docstrings, consistent with the surrounding code. |
|
Confirmed the composite-partition read-after-write issue at [P1] Make the writer and reader agree on the new composite partition layout
The commit succeeds, then readback raises I ran the added DATE tests plus file-store-commit and Mosaic writer regressions: 39 tests and 2 subtests passed; changed files passed Flake8. The current composite test asserts only the path and misses the failing readback. Please make planning resolve the actual new layout, or use a fully canonical layout with historical compatibility, and add write → commit → read assertions for composite DATE partitions with values requiring escaping. Preserve historical/new-file coexistence while doing so. |
…on-date-partition-compat # Conflicts: # paimon-python/pypaimon/tests/mosaic_writer_options_test.py # paimon-python/pypaimon/write/file_store_commit.py # paimon-python/pypaimon/write/writer/data_writer.py
|
Thanks for the update. I independently tested The OSS performance regression is a concern. With 128 small Parquet files, median planning + full-read time increased from 1.71 to 2.24 seconds (+31%) for a DATE composite partition, and from 1.47 to 2.47 seconds (+68%) for an escaped STRING partition without DATE. These were three measured runs after warm-up on the same frozen files. Local full-read performance was roughly unchanged. The head adds path-existence checks even for canonical files, so the description's claim of no additional lookups needs updating. I do not think this is ready to merge. I closed my own attempt in #10045 because these compatibility and performance tradeoffs were unresolved. Please align on the design before making further code changes: which version combinations we support, how readers locate historical files, and what lookup cost is acceptable. Adding another fallback for each reported failure is not a sufficient compatibility strategy, especially when it slows down tables that already use canonical paths. Once the design is agreed, please implement it and provide reproducible correctness and performance results. Please also change the added Chinese comments and docstrings to English, consistent with the surrounding code. |
|
Thanks for the detailed cross-version results and OSS measurements. I agree this PR is not ready to merge, and I will hold further code changes until we align on the compatibility scope and performance trade-off. On the current head ( I appreciate your reported 256-table cross-version mixed-writer matrix; I have not independently reproduced it. The historical-layout test currently in this PR constructs the old directory layout synthetically rather than running each supported historical client version. The support boundary for Java readers of historical Python layouts also remains unresolved; your report indicates that some of those combinations are not currently supported. I also need to correct the performance statement in the PR description. The current change adds canonical-path Before I make further code changes, could we agree on:
Once we agree on these boundaries and the performance criterion, I can update the compatibility tests and performance description to match. I will also keep new comments and docstrings in English, consistent with the surrounding code. |
Fixes #10034
Summary
Compatibility coverage
partition.legacy-name=trueandfalse, includingregion=a/b.paimon-python/dev/run_mixed_tests.sh.Performance assessment
This changes write-path selection and does not add reader fallback logic. The existing escaped-partition reader path already performs one canonical-path existence check per data-file metadata entry; this change does not add another lookup. No remote/object-store benchmark was run locally.
Verification
file:external-path fixture is not Windows-compatible.ServiceLoaderNPE inCodeGenUtils) before reaching the partition assertions. The PR's Java workflow is currently skipped, so the mixed Java/Python chain still needs an environment where it can be run.