Skip to content

[python] Align DATE partition paths with Java - #10319

Open
zhengguangzhuo wants to merge 7 commits into
apache:masterfrom
zhengguangzhuo:zhengguangzhuo/paimon-date-partition-compat
Open

zhengguangzhuo wants to merge 7 commits into
apache:masterfrom
zhengguangzhuo:zhengguangzhuo/paimon-date-partition-compat

Conversation

@zhengguangzhuo

@zhengguangzhuo zhengguangzhuo commented Sep 29, 2026 •

Copy link
Copy Markdown

Fixes #10034

Summary

  • Write PyPaimon data files with the canonical partition-path encoding expected by readers, including DATE compatibility and escaped partition values.
  • Reconstruct duplicate-commit file paths using the same canonical layout.

Compatibility coverage

  • Verify composite DATE read-after-write with partition.legacy-name=true and false, including region=a/b.
  • Keep historical Python-layout files and newly written canonical files in the same logical partition and bucket; verify append reads both rows and rollback still reads the historical row.
  • Add Java writer → Python reader/append → Java reader coverage for both naming modes, including a Java read after rollback. Register these cases in 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

  • Targeted Python regressions: 40 passed; one unchanged upstream abort test was deselected because its file: external-path fixture is not Windows-compatible.
  • Maven compiled the Java E2E test, but local execution on Windows with Temurin 8 stopped in generated projection loading (ServiceLoader NPE in CodeGenUtils) 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.

@wangzhigang1999

Copy link
Copy Markdown
Contributor

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 partition.legacy-name=true, day=1970-01-02, and region=a/b, this version writes to day=1/region=a/b/..., but the Python reader cannot resolve that mixed-format path and fails with FileNotFoundError. The added composite-partition test checks the path without verifying readback.

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.

@JingsongLi

Copy link
Copy Markdown
Contributor

Confirmed the composite-partition read-after-write issue at 7557c7e144 with an actual table write, commit and read. The DATE compatibility problem in #10034 is worth fixing, but this version should be blocked from production.

[P1] Make the writer and reader agree on the new composite partition layout

pypaimon/utils/file_store_path_factory.py:247-273 converts only DATE components, creating a third layout that neither reader path resolves. With default partition.legacy-name=true, day=1970-01-02 and region=a/b:

New writer:             day=1/region=a/b/bucket-0/data-....parquet
Historical reader path: day=1970-01-02/region=a/b/bucket-0/data-....parquet
Canonical reader path:  day=1/region=a%2Fb/bucket-0/data-....parquet

The commit succeeds, then readback raises FileNotFoundError. An in-process control retaining the previous writer path reads the same row successfully; partition.legacy-name=false also reads back successfully. This confirms a newly introduced failure rather than an existing unreadable table.

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.

@Akash3121 Akash3121 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

…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
@wangzhigang1999

Copy link
Copy Markdown
Contributor

Thanks for the update. I independently tested 1156834035 against 80c5ab9593. The PR's Python reader passed all 256 tables in my cross-version, mixed-writer matrix, including the original composite-partition failure. Java still cannot read some historical Python layouts; that is an existing limitation, but the compatibility boundary needs to be explicit.

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.

@zhengguangzhuo

Copy link
Copy Markdown
Author

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 (0e1ac11), CI run 37882502997 completed successfully. All Python jobs passed; Native CI reported 9,586 passed and 112 skipped. The standalone Java job was skipped. However, the Python 3.10 job did run the mixed Java-Python suite: the Java composite DATE write, Python composite DATE read and append, and Java read-after-write/rollback tests passed for both partition naming modes. This validates those current-version combinations, but not the full historical-client matrix or every historical Python layout.

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 exists_batch checks when the canonical and legacy bucket paths differ, followed by a historical-path lookup when the canonical candidates are absent. exists_batch batches paths at the FileIO API, but that alone does not establish the number or cost of metadata requests made by an object-store backend. I have not independently reproduced your OSS benchmark, and local-filesystem timings are not comparable evidence. I will not use them to dismiss the reported regressions of +31% and +68%.

Before I make further code changes, could we agree on:

  1. The exact Java/Python versions and writer-to-reader combinations in scope, for both partition.legacy-name modes, including mixed-writer append and rollback scenarios—and which combinations are explicitly out of scope.
  2. Which historical Python layouts must be readable, and by which readers, especially whether Java reading those layouts is a requirement.
  3. An acceptable object-store metadata-lookup/latency budget and a reproducible benchmark setup. In particular, should reads of an already-canonical layout incur any existence probes?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] PyPaimon DATE partition naming differs from Java

4 participants