Skip to content

Commit 09b026f

Browse files
scovichscovich
authored
feat: Define and use constants for protocol (3,7) (delta-io#1917)
## What changes are proposed in this pull request? Reader protocol 3 and writer protocol 7 are special, because they introduce the concept of table features (which require feature lists where lower protocols forbid them). But they are _also_ special because they're currently the highest protocol version numbers that kernel supports. Replace a bunch of magic values in the code with proper constants to make clearer what's going on. If/when the Delta spec bumps the max protocol version number, we'll hopefully have fewer ambiguous `3` and `7` values that need careful auditing. ## How was this change tested? Mechanical replacement of literals with same-valued constants. Compilation and unit tests cover it.
1 parent 84606f7 commit 09b026f

4 files changed

Lines changed: 74 additions & 49 deletions

File tree

‎kernel/src/actions/mod.rs‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ use std::sync::{Arc, LazyLock};
77
use self::deletion_vector::DeletionVectorDescriptor;
88
use crate::expressions::{MapData, Scalar, StructData};
99
use crate::schema::{DataType, MapType, SchemaRef, StructField, StructType, ToSchema as _};
10-
use crate::table_features::{FeatureType, IntoTableFeature, TableFeature};
10+
use crate::table_features::{
11+
FeatureType, IntoTableFeature, TableFeature, TABLE_FEATURES_MIN_READER_VERSION,
12+
TABLE_FEATURES_MIN_WRITER_VERSION,
13+
};
1114
use crate::table_properties::TableProperties;
1215
use crate::utils::require;
1316
use crate::{
@@ -435,7 +438,12 @@ impl Protocol {
435438
reader_features: impl IntoIterator<Item = impl IntoTableFeature>,
436439
writer_features: impl IntoIterator<Item = impl IntoTableFeature>,
437440
) -> DeltaResult<Self> {
438-
Self::try_new(3, 7, Some(reader_features), Some(writer_features))
441+
Self::try_new(
442+
TABLE_FEATURES_MIN_READER_VERSION,
443+
TABLE_FEATURES_MIN_WRITER_VERSION,
444+
Some(reader_features),
445+
Some(writer_features),
446+
)
439447
}
440448

441449
/// Try to create a new legacy Protocol instance with the given reader/writer versions
@@ -463,7 +471,7 @@ impl Protocol {
463471
let writer_features = parse_features(writer_features);
464472

465473
// The protocol states that Reader features may be present if and only if the min_reader_version is 3
466-
if min_reader_version == 3 {
474+
if min_reader_version == TABLE_FEATURES_MIN_READER_VERSION {
467475
require!(
468476
reader_features.is_some(),
469477
Error::invalid_protocol(
@@ -480,7 +488,7 @@ impl Protocol {
480488
}
481489

482490
// The protocol states that Writer features may be present if and only if the min_writer_version is 7
483-
if min_writer_version == 7 {
491+
if min_writer_version == TABLE_FEATURES_MIN_WRITER_VERSION {
484492
require!(
485493
writer_features.is_some(),
486494
Error::invalid_protocol(

‎kernel/src/snapshot.rs‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -772,6 +772,9 @@ mod tests {
772772
use crate::log_segment::LogSegment;
773773
use crate::parquet::arrow::ArrowWriter;
774774
use crate::path::ParsedLogPath;
775+
use crate::table_features::{
776+
TABLE_FEATURES_MIN_READER_VERSION, TABLE_FEATURES_MIN_WRITER_VERSION,
777+
};
775778
use crate::utils::test_utils::{assert_result_error_with_message, string_array_to_engine_data};
776779
use delta_kernel::actions::DomainMetadata;
777780

@@ -798,13 +801,13 @@ mod tests {
798801
let mut protocol = json!({
799802
"protocol": {
800803
"minReaderVersion": reader_version,
801-
"minWriterVersion": 7,
804+
"minWriterVersion": TABLE_FEATURES_MIN_WRITER_VERSION,
802805
"writerFeatures": ["inCommitTimestamp"]
803806
}
804807
});
805808

806-
// Only include readerFeatures if minReaderVersion >= 3
807-
if reader_version >= 3 {
809+
// Only include readerFeatures if minReaderVersion >= table-features minimum.
810+
if reader_version >= TABLE_FEATURES_MIN_READER_VERSION as u32 {
808811
protocol["protocol"]["readerFeatures"] = json!([]);
809812
}
810813

@@ -1603,8 +1606,8 @@ mod tests {
16031606
let commit_data = [
16041607
json!({
16051608
"protocol": {
1606-
"minReaderVersion": 3,
1607-
"minWriterVersion": 7,
1609+
"minReaderVersion": TABLE_FEATURES_MIN_READER_VERSION,
1610+
"minWriterVersion": TABLE_FEATURES_MIN_WRITER_VERSION,
16081611
"readerFeatures": [],
16091612
"writerFeatures": ["inCommitTimestamp"]
16101613
}
@@ -1650,7 +1653,7 @@ mod tests {
16501653
let engine = DefaultEngineBuilder::new(store.clone()).build();
16511654

16521655
let commit_data = [
1653-
create_protocol(true, Some(3)),
1656+
create_protocol(true, Some(TABLE_FEATURES_MIN_READER_VERSION as u32)),
16541657
create_metadata(
16551658
Some("test_id"),
16561659
Some("{\"type\":\"struct\",\"fields\":[]}"),
@@ -1683,7 +1686,7 @@ mod tests {
16831686

16841687
// Create initial commit with ICT enabled
16851688
let commit_data = [
1686-
create_protocol(true, Some(3)),
1689+
create_protocol(true, Some(TABLE_FEATURES_MIN_READER_VERSION as u32)),
16871690
create_metadata(
16881691
Some("test_id"),
16891692
Some("{\"type\":\"struct\",\"fields\":[]}"),
@@ -1738,7 +1741,7 @@ mod tests {
17381741
// Create 00000000000000000000.json with ICT enabled
17391742
let commit0_data = [
17401743
create_commit_info(1587968586154, None),
1741-
create_protocol(true, Some(3)),
1744+
create_protocol(true, Some(TABLE_FEATURES_MIN_READER_VERSION as u32)),
17421745
create_metadata(
17431746
Some("5fba94ed-9794-4965-ba6e-6ee3c0d22af9"),
17441747
Some("{\"type\":\"struct\",\"fields\":[{\"name\":\"id\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}},{\"name\":\"val\",\"type\":\"string\",\"nullable\":true,\"metadata\":{}}]}"),
@@ -1752,7 +1755,7 @@ mod tests {
17521755
// Create 00000000000000000001.checkpoint.parquet
17531756
let checkpoint_data = [
17541757
create_commit_info(1587968586154, None),
1755-
create_protocol(true, Some(3)),
1758+
create_protocol(true, Some(TABLE_FEATURES_MIN_READER_VERSION as u32)),
17561759
create_metadata(
17571760
Some("5fba94ed-9794-4965-ba6e-6ee3c0d22af9"),
17581761
Some("{\"type\":\"struct\",\"fields\":[{\"name\":\"id\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}},{\"name\":\"val\",\"type\":\"string\",\"nullable\":true,\"metadata\":{}}]}"),

‎kernel/src/table_configuration.rs‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ use crate::table_features::{
2323
column_mapping_mode, get_any_level_column_physical_name, validate_schema_column_mapping,
2424
validate_timestamp_ntz_feature_support, ColumnMappingMode, EnablementCheck, FeatureInfo,
2525
FeatureRequirement, FeatureType, KernelSupport, Operation, TableFeature,
26-
LEGACY_READER_FEATURES, LEGACY_WRITER_FEATURES,
26+
LEGACY_READER_FEATURES, LEGACY_WRITER_FEATURES, MAX_VALID_READER_VERSION,
27+
MAX_VALID_WRITER_VERSION, TABLE_FEATURES_MIN_READER_VERSION, TABLE_FEATURES_MIN_WRITER_VERSION,
2728
};
2829
use crate::table_properties::TableProperties;
2930
use crate::utils::require;
@@ -432,7 +433,7 @@ impl TableConfiguration {
432433
/// For legacy protocol (v1-2), infers features from the version number.
433434
fn get_enabled_reader_features(&self) -> Vec<TableFeature> {
434435
match self.protocol.min_reader_version() {
435-
3 => {
436+
TABLE_FEATURES_MIN_READER_VERSION => {
436437
// Table features reader: use explicit reader_features list
437438
self.protocol
438439
.reader_features()
@@ -460,7 +461,7 @@ impl TableConfiguration {
460461
/// For legacy protocol (v1-6), infers features from the version number.
461462
fn get_enabled_writer_features(&self) -> Vec<TableFeature> {
462463
match self.protocol.min_writer_version() {
463-
7 => {
464+
TABLE_FEATURES_MIN_WRITER_VERSION => {
464465
// Table features writer: use explicit writer_features list
465466
self.protocol
466467
.writer_features()
@@ -498,8 +499,8 @@ impl TableConfiguration {
498499

499500
/// Internal helper for read operations (Scan, Cdf)
500501
fn ensure_read_supported(&self, operation: Operation) -> DeltaResult<()> {
501-
// Version check: kernel supports reader versions 1-3
502-
if self.protocol.min_reader_version() > 3 {
502+
// Version check: kernel supports reader versions 1..=MAX_VALID_READER_VERSION
503+
if self.protocol.min_reader_version() > MAX_VALID_READER_VERSION {
503504
return Err(Error::unsupported(format!(
504505
"Unsupported minimum reader version {}",
505506
self.protocol.min_reader_version()
@@ -516,8 +517,8 @@ impl TableConfiguration {
516517

517518
/// Internal helper for write operations
518519
fn ensure_write_supported(&self) -> DeltaResult<()> {
519-
// Version check: kernel supports writer versions 1-7
520-
if self.protocol.min_writer_version() > 7 {
520+
// Version check: kernel supports writer versions 1..=MAX_VALID_WRITER_VERSION
521+
if self.protocol.min_writer_version() > MAX_VALID_WRITER_VERSION {
521522
return Err(Error::unsupported(format!(
522523
"Unsupported minimum writer version {}",
523524
self.protocol.min_writer_version()
@@ -604,13 +605,13 @@ impl TableConfiguration {
604605
/// Returns true if the protocol uses legacy reader version (< 3)
605606
#[allow(dead_code)]
606607
fn is_legacy_reader_version(&self) -> bool {
607-
self.protocol.min_reader_version() < 3
608+
self.protocol.min_reader_version() < TABLE_FEATURES_MIN_READER_VERSION
608609
}
609610

610611
/// Returns true if the protocol uses legacy writer version (< 7)
611612
#[allow(dead_code)]
612613
fn is_legacy_writer_version(&self) -> bool {
613-
self.protocol.min_writer_version() < 7
614+
self.protocol.min_writer_version() < TABLE_FEATURES_MIN_WRITER_VERSION
614615
}
615616

616617
/// Helper to check if a feature is present in a feature list.
@@ -709,6 +710,7 @@ mod test {
709710
use crate::table_features::ColumnMappingMode;
710711
use crate::table_features::{
711712
EnablementCheck, FeatureInfo, FeatureType, KernelSupport, Operation, TableFeature,
713+
TABLE_FEATURES_MIN_READER_VERSION, TABLE_FEATURES_MIN_WRITER_VERSION,
712714
};
713715
use crate::table_properties::TableProperties;
714716
use crate::utils::test_utils::{
@@ -726,7 +728,12 @@ mod test {
726728
props_to_enable: &[(&str, &str)],
727729
features: &[TableFeature],
728730
) -> TableConfiguration {
729-
create_mock_table_config_with_version(props_to_enable, Some(features), 3, 7)
731+
create_mock_table_config_with_version(
732+
props_to_enable,
733+
Some(features),
734+
TABLE_FEATURES_MIN_READER_VERSION,
735+
TABLE_FEATURES_MIN_WRITER_VERSION,
736+
)
730737
}
731738

732739
fn create_mock_table_config_with_version(
@@ -767,9 +774,10 @@ mod test {
767774
.filter(|f| f.feature_type() == FeatureType::ReaderWriter);
768775
(
769776
// Only add reader_features if reader >= 3 (non-legacy reader mode)
770-
(min_reader_version >= 3).then_some(reader_features),
777+
(min_reader_version >= TABLE_FEATURES_MIN_READER_VERSION)
778+
.then_some(reader_features),
771779
// Only add writer_features if writer >= 7 (non-legacy writer mode)
772-
(min_writer_version >= 7).then_some(features),
780+
(min_writer_version >= TABLE_FEATURES_MIN_WRITER_VERSION).then_some(features),
773781
)
774782
} else {
775783
(None, None)

‎kernel/src/table_features/mod.rs‎

Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ pub(crate) use timestamp_ntz::validate_timestamp_ntz_feature_support;
1919
mod column_mapping;
2020
mod timestamp_ntz;
2121

22+
/// Maximum reader protocol version that the kernel can handle.
23+
pub const MAX_VALID_READER_VERSION: i32 = 3;
24+
25+
/// Maximum writer protocol version that the kernel can handle.
26+
pub const MAX_VALID_WRITER_VERSION: i32 = 7;
27+
2228
/// Minimum reader version for tables that use table features.
2329
/// When set to 3, the protocol requires an explicit `readerFeatures` array.
2430
pub const TABLE_FEATURES_MIN_READER_VERSION: i32 = 3;
@@ -446,8 +452,8 @@ static CLUSTERED_TABLE_INFO: FeatureInfo = FeatureInfo {
446452
#[allow(dead_code)]
447453
static MATERIALIZE_PARTITION_COLUMNS_INFO: FeatureInfo = FeatureInfo {
448454
name: "materializePartitionColumns",
449-
min_reader_version: 3,
450-
min_writer_version: 7,
455+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
456+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
451457
feature_type: FeatureType::Writer,
452458
feature_requirements: &[],
453459
kernel_support: KernelSupport::Supported,
@@ -457,8 +463,8 @@ static MATERIALIZE_PARTITION_COLUMNS_INFO: FeatureInfo = FeatureInfo {
457463
#[allow(dead_code)]
458464
static CATALOG_MANAGED_INFO: FeatureInfo = FeatureInfo {
459465
name: "catalogManaged",
460-
min_reader_version: 3,
461-
min_writer_version: 7,
466+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
467+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
462468
feature_type: FeatureType::ReaderWriter,
463469
feature_requirements: &[],
464470
#[cfg(feature = "catalog-managed")]
@@ -476,8 +482,8 @@ static CATALOG_MANAGED_INFO: FeatureInfo = FeatureInfo {
476482
#[allow(dead_code)]
477483
static CATALOG_OWNED_PREVIEW_INFO: FeatureInfo = FeatureInfo {
478484
name: "catalogOwned-preview",
479-
min_reader_version: 3,
480-
min_writer_version: 7,
485+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
486+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
481487
feature_type: FeatureType::ReaderWriter,
482488
feature_requirements: &[],
483489
#[cfg(feature = "catalog-managed")]
@@ -514,8 +520,8 @@ static COLUMN_MAPPING_INFO: FeatureInfo = FeatureInfo {
514520
#[allow(dead_code)]
515521
static DELETION_VECTORS_INFO: FeatureInfo = FeatureInfo {
516522
name: "deletionVectors",
517-
min_reader_version: 3,
518-
min_writer_version: 7,
523+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
524+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
519525
feature_type: FeatureType::ReaderWriter,
520526
feature_requirements: &[],
521527
// We support writing to tables with DeletionVectors enabled, but we never write DV files
@@ -529,8 +535,8 @@ static DELETION_VECTORS_INFO: FeatureInfo = FeatureInfo {
529535
#[allow(dead_code)]
530536
static TIMESTAMP_WITHOUT_TIMEZONE_INFO: FeatureInfo = FeatureInfo {
531537
name: "timestampNtz",
532-
min_reader_version: 3,
533-
min_writer_version: 7,
538+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
539+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
534540
feature_type: FeatureType::ReaderWriter,
535541
feature_requirements: &[],
536542
kernel_support: KernelSupport::Supported,
@@ -540,8 +546,8 @@ static TIMESTAMP_WITHOUT_TIMEZONE_INFO: FeatureInfo = FeatureInfo {
540546
#[allow(dead_code)]
541547
static TYPE_WIDENING_INFO: FeatureInfo = FeatureInfo {
542548
name: "typeWidening",
543-
min_reader_version: 3,
544-
min_writer_version: 7,
549+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
550+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
545551
feature_type: FeatureType::ReaderWriter,
546552
feature_requirements: &[],
547553
kernel_support: KernelSupport::Custom(|_, _, op| match op {
@@ -556,8 +562,8 @@ static TYPE_WIDENING_INFO: FeatureInfo = FeatureInfo {
556562
#[allow(dead_code)]
557563
static TYPE_WIDENING_PREVIEW_INFO: FeatureInfo = FeatureInfo {
558564
name: "typeWidening-preview",
559-
min_reader_version: 3,
560-
min_writer_version: 7,
565+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
566+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
561567
feature_type: FeatureType::ReaderWriter,
562568
feature_requirements: &[],
563569
kernel_support: KernelSupport::Custom(|_, _, op| match op {
@@ -572,8 +578,8 @@ static TYPE_WIDENING_PREVIEW_INFO: FeatureInfo = FeatureInfo {
572578
#[allow(dead_code)]
573579
static V2_CHECKPOINT_INFO: FeatureInfo = FeatureInfo {
574580
name: "v2Checkpoint",
575-
min_reader_version: 3,
576-
min_writer_version: 7,
581+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
582+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
577583
feature_type: FeatureType::ReaderWriter,
578584
feature_requirements: &[],
579585
kernel_support: KernelSupport::Supported,
@@ -583,8 +589,8 @@ static V2_CHECKPOINT_INFO: FeatureInfo = FeatureInfo {
583589
#[allow(dead_code)]
584590
static VACUUM_PROTOCOL_CHECK_INFO: FeatureInfo = FeatureInfo {
585591
name: "vacuumProtocolCheck",
586-
min_reader_version: 3,
587-
min_writer_version: 7,
592+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
593+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
588594
feature_type: FeatureType::ReaderWriter,
589595
feature_requirements: &[],
590596
kernel_support: KernelSupport::Supported,
@@ -594,8 +600,8 @@ static VACUUM_PROTOCOL_CHECK_INFO: FeatureInfo = FeatureInfo {
594600
#[allow(dead_code)]
595601
static VARIANT_TYPE_INFO: FeatureInfo = FeatureInfo {
596602
name: "variantType",
597-
min_reader_version: 3,
598-
min_writer_version: 7,
603+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
604+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
599605
feature_type: FeatureType::ReaderWriter,
600606
feature_requirements: &[],
601607
kernel_support: KernelSupport::Supported,
@@ -605,8 +611,8 @@ static VARIANT_TYPE_INFO: FeatureInfo = FeatureInfo {
605611
#[allow(dead_code)]
606612
static VARIANT_TYPE_PREVIEW_INFO: FeatureInfo = FeatureInfo {
607613
name: "variantType-preview",
608-
min_reader_version: 3,
609-
min_writer_version: 7,
614+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
615+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
610616
feature_type: FeatureType::ReaderWriter,
611617
feature_requirements: &[],
612618
kernel_support: KernelSupport::Supported,
@@ -616,8 +622,8 @@ static VARIANT_TYPE_PREVIEW_INFO: FeatureInfo = FeatureInfo {
616622
#[allow(dead_code)]
617623
static VARIANT_SHREDDING_PREVIEW_INFO: FeatureInfo = FeatureInfo {
618624
name: "variantShredding-preview",
619-
min_reader_version: 3,
620-
min_writer_version: 7,
625+
min_reader_version: TABLE_FEATURES_MIN_READER_VERSION,
626+
min_writer_version: TABLE_FEATURES_MIN_WRITER_VERSION,
621627
feature_type: FeatureType::ReaderWriter,
622628
feature_requirements: &[],
623629
kernel_support: KernelSupport::Supported,

0 commit comments

Comments
 (0)