Akash3121 commented on code in PR #10166:
URL: https://github.com/apache/paimon/pull/10166#discussion_r4097389842
##########
paimon-python/pypaimon/tests/test_sequence_field_read.py:
##########
@@ -301,7 +302,8 @@ def test_missing_sequence_field_rejected(self):
"""
table = self._create_pk_table(
Review Comment:
This expectation currently fails under the required Native CI configuration
because validation was added only to `KeyValueDataWriter`, which is never
constructed when `WriteBuilder` selects the native deduplicate writer. Native
CI shows the same bypass for missing, duplicate and empty sequence fields and
for cross-partition updates: all four writes complete without the expected
`ValueError`. Please perform table-level sequence validation before
native/Python backend selection (or add equivalent native validation); marking
these tests as Python-only would hide a real behavior difference.
##########
paimon-python/pypaimon/write/writer/key_value_data_writer.py:
##########
@@ -45,6 +45,15 @@ def __init__(self, table, partition, bucket, max_seq_number,
changelog_producer=ChangelogProducer.NONE):
super().__init__(table, partition, bucket, max_seq_number,
options, write_cols, changelog_producer)
+ if self.options.sequence_field():
Review Comment:
This constructor is reached only after `TableWrite.write_arrow_batch` calls
`row_key_extractor.extract_partition_bucket_groups`. With `bucket=-1`, that
call records new mappings in `DynamicBucketIndexMaintainer` before this
validation can reject a missing/duplicate/unsupported sequence field. If the
caller catches the error and invokes `prepare_commit`, it can receive
index-only commit messages and create HASH index files for rows that were never
accepted. Please move validation ahead of row-key extraction and add a
dynamic-bucket regression asserting that a rejected write leaves
`prepare_commit` empty and creates no index files.
##########
paimon-python/pypaimon/write/writer/key_value_data_writer.py:
##########
@@ -286,6 +295,11 @@ def _sort_by_primary_key(
# Table, so this serves both the per-batch entry path (legacy)
# and the buffer-wide sort path (used by ``_flush_all``).
sort_keys = [(key, 'ascending') for key in self.trimmed_primary_keys]
+ sequence_fields = self.options.sequence_field()
Review Comment:
This does not match the read-side comparator for FLOAT/DOUBLE NaNs, so the
same rows can still produce different winners depending on grouping.
`null_placement='at_start'` also places NaNs before finite values in Arrow. For
rows written as `(seq=1.0, value='finite-first')` followed by `(seq=NaN,
value='nan-last')`, one buffered write sorts NaN first and therefore keeps
finite-first . Across separate commits, `_row_field_comparator` treats NaN and
`1.0` as equal because both `<` and `>` are false, then the generated/file
sequence number breaks the tie, so the later `nan-last` row wins. Please define
one NaN ordering shared by the writer and `builtin_seq_comparator` (and aligned
with Java), and add batch/chunk/commit regressions for NaN in both sort
directions.
--
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]
