[Spark][Version Checksum] Read Protocol, Metadata, and ICT directly from the Checksum during Snapshot construction #3920
Conversation
dhruvarya-db
force-pushed
the
checksum-read-pnm
branch
from
e868303 to
3d69df7
Compare
dhruvarya-db
requested review from
ryan-johnson-databricks
and removed request for
ryan-johnson-databricks
dhruvarya-db
force-pushed
the
checksum-read-pnm
branch
from
2551290 to
af9c14c
Compare
| if (spark.sessionState.conf.getConf( | ||
| DeltaSQLConf.USE_PROTOCOL_AND_METADATA_FROM_CHECKSUM_ENABLED)) { |
There was a problem hiding this comment.
Is it useful/important to make this check conditional, where it was previously unconditional?
What is the rationale?
There was a problem hiding this comment.
I thought about this again, and this does not make much sense. Removed
| log"protocol:${MDC(DeltaLogKeys.PROTOCOL, protocolStr)} " + | ||
| log"metadata:${MDC(DeltaLogKeys.DELTA_METADATA, metadataStr)}") |
There was a problem hiding this comment.
Why is the one called PROTOCOL but the other DELTA_METADATA? Why not just METADATA?
There was a problem hiding this comment.
update: There are already several METADATA_xxx keys, I guess this avoids ambiguity?
| // Make sure that we don't retrieve the time from the CRC. | ||
| withSQLConf(DeltaSQLConf.DELTA_WRITE_CHECKSUM_ENABLED.key -> "false") { | ||
| withTempDir { tempDir => |
There was a problem hiding this comment.
odd (lack of) indentation here?
(several more below)
There was a problem hiding this comment.
Fixed the indentation. I wanted to keep the diff cleaner but given that the content itself has been reviewed, that should not matter anymore
There was a problem hiding this comment.
FYI - Indentation changes are easily hidden during review:
dhruvarya-db
force-pushed
the
checksum-read-pnm
branch
from
8336241 to
445d4e7
Compare
dhruvarya-db
force-pushed
the
checksum-read-pnm
branch
from
445d4e7 to
b7f4845
Compare
Which Delta project/connector is this regarding?
Description
Stacked over #3907.
This PR makes the Checksum (if available) the source of truth for Protocol, Metadata, ICT during snapshot construction. This helps us avoid a Spark query and improves performance.
How was this patch tested?
Added some test cases to existing suites
Does this PR introduce any user-facing changes?
No