[Kernel] Support enabling RowTracking #4582
Conversation
KaiqiJinWow
changed the title
KaiqiJinWow
requested review from
allisonport-db and
vkorukanti
and removed request for
allisonport-db and
vkorukanti
KaiqiJinWow
changed the title
| boolean dataChange, | ||
| Map<String, String> tags) { | ||
| Map<String, String> tags, | ||
| Optional<Long> baseRowId, |
There was a problem hiding this comment.
Need to update once the IcebergCompatWriterV3 introduced. We only allow passing these row tracking infos on generateIcebergCompatWriterV3AddAction.
KaiqiJinWow
force-pushed
the
update_kernel_row_id
branch
from
ed1fe1a to
d8dc4e4
Compare
KaiqiJinWow
force-pushed
the
update_kernel_row_id
branch
from
d8dc4e4 to
dc802e7
Compare
| public static final MaterializedRowTrackingColumn ROW_ID = | ||
| new MaterializedRowTrackingColumn( | ||
| TableConfig.MATERIALIZED_ROW_ID_COLUMN_NAME, "_row-id-col-"); | ||
| TableConfig.MATERIALIZED_ROW_ID_COLUMN_NAME, "_row-id-col-", 2147483540L); |
There was a problem hiding this comment.
put a reference to iceberg spec
| stat -> | ||
| fieldMap.put( | ||
| RemoveFile.FULL_SCHEMA.indexOf("stats"), stat.serializeAsJson(physicalSchema))); | ||
| baseRowId.ifPresent(id -> fieldMap.put(RemoveFile.FULL_SCHEMA.indexOf("baseRowId"), id)); |
There was a problem hiding this comment.
do we have a counter part of this for create AddFile ?
| TableConfig.MATERIALIZED_ROW_COMMIT_VERSION_COLUMN_NAME, "_row-commit-version-col-"); | ||
| TableConfig.MATERIALIZED_ROW_COMMIT_VERSION_COLUMN_NAME, | ||
| "_row-commit-version-col-", | ||
| 2147483539L); |
There was a problem hiding this comment.
do we always apply this id regardless of iceberg compat ? I think we should only do this for v3.
| }); | ||
|
|
||
| if (currRowIdHighWatermark.get() != prevRowIdHighWatermark) { | ||
| // If the client has explicitly provided a row ID high watermark, we should use that value |
There was a problem hiding this comment.
nit: client is ambiguous... should it be transaction builder ?
KaiqiJinWow
changed the title
| }); | ||
|
|
||
| if (currRowIdHighWatermark.get() != prevRowIdHighWatermark) { | ||
| // If the client has explicitly provided a row ID high watermark, we should use that value |
There was a problem hiding this comment.
@KaiqiJinWow
Can you provide more context on this change, I'm not sure I see why a client would need to provide an explicit high-water mark instead of relying on kernel automatically raising it on write
In particular:
- how is this going to be used. I assume this has to do with Iceberg compat, that would be useful to cover in the PR description / title
- Where is the provided high-water mark coming from?
There was a problem hiding this comment.
Hi This PR is staled, please check the merged one #4856
3 participants
Which Delta project/connector is this regarding?
Description
This PR is trying to unblock RowTracking feature in Kernel, which includes
delta.rowTracking.materializedRowIdColumnNameanddelta.rowTracking.materializedRowCommitVersionColumnName.IcebergCompatWriterV1AddActionandIcebergCompatWriterV1RemoveActionHow was this patch tested?
Existing and newly added unit tests.
Does this PR introduce any user-facing changes?