Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
make test work
  • Loading branch information
kevinjqliu committed Apr 28, 2024
commit ecec57e1b823a29a76f1c15477cc6958e70105ee
3 changes: 2 additions & 1 deletion pyiceberg/table/snapshots.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ def __eq__(self, other: Any) -> bool:
class Snapshot(IcebergBaseModel):
snapshot_id: int = Field(alias="snapshot-id")
parent_snapshot_id: Optional[int] = Field(alias="parent-snapshot-id", default=None)
sequence_number: Optional[int] = Field(alias="sequence-number", default=None)
# cannot import `INITIAL_SEQUENCE_NUMBER` due to circular import
sequence_number: Optional[int] = Field(alias="sequence-number", default=0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason the default value for the sequence number has to be changed to 0 as opposed to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the spec, https://iceberg.apache.org/spec/#version-2

Snapshot JSON:
sequence-number was added and is required; default to 0 when reading v1 metadata

Also added this in the PR description

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevinjqliu Thanks for spotting this! We definitely need to read snapshot.sequence_number as 0 for v1. However, as we have observed in the test outcome, making sequence_number default to 0 here leads to sequence_number=0 be written to version 1 table metada's snapshots, which is not allowed by spec:

Writing v1 metadata:
Snapshot field sequence-number should not be written

I think we may need a new field_serializer in TableMetadataCommonFields class or some other ways to correct the behavior on write. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I missed the part about the V1 spec. Following your suggestion, I added a field_serializer for TableMetadataCommonFields. This will ensure that the Snapshot pydantic object will not have the sequence-number field for V1 format

timestamp_ms: int = Field(alias="timestamp-ms", default_factory=lambda: int(time.time() * 1000))
manifest_list: Optional[str] = Field(
alias="manifest-list", description="Location of the snapshot's manifest list file", default=None
Expand Down
16 changes: 14 additions & 2 deletions tests/integration/test_inspect_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,8 @@ def check_pyiceberg_df_equals_spark_df(df: pa.Table, spark_df: DataFrame) -> Non
def test_inspect_metadata_log_entries(
spark: SparkSession, session_catalog: Catalog, arrow_table_with_null: pa.Table, format_version: int
) -> None:
from pandas.testing import assert_frame_equal

identifier = "default.table_metadata_log_entries"
tbl = _create_table(session_catalog, identifier, properties={"format-version": format_version})

Expand All @@ -463,5 +465,15 @@ def test_inspect_metadata_log_entries(
spark_df = spark.sql(f"SELECT * FROM {identifier}.metadata_log_entries")
lhs = df.to_pandas()
rhs = spark_df.toPandas()
breakpoint()
assert lhs.equals(rhs), lhs.compare(rhs)

# Timestamp in the last row of `metadata_log_entries` table is the based on when the table was read
# Therefore, the timestamp for pyiceberg dataframe and spark dataframe will be different
left_before_last, left_last = lhs[:-1], lhs[-1:]
right_before_last, right_last = rhs[:-1], rhs[-1:]

assert_frame_equal(left_before_last, right_before_last, check_dtype=False)
for column in df.column_names:
for left, right in zip(left_last[column], right_last[column]):
if column == 'timestamp':
continue
assert left == right, f"Difference in column {column}: {left} != {right}"