Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: always include the Invariants writerFeature when upgrading to v7 #2957

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 4 additions & 1 deletion crates/core/src/kernel/models/actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,10 @@ impl Protocol {
/// Enable timestamp_ntz in the protocol
pub fn enable_timestamp_ntz(mut self) -> Protocol {
self = self.with_reader_features(vec![ReaderFeatures::TimestampWithoutTimezone]);
self = self.with_writer_features(vec![WriterFeatures::TimestampWithoutTimezone]);
self = self.with_writer_features(vec![
WriterFeatures::TimestampWithoutTimezone,
WriterFeatures::Invariants,
]);
self
}
}
Expand Down
43 changes: 42 additions & 1 deletion crates/core/src/operations/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,9 @@ impl CreateBuilder {
Protocol {
min_reader_version: 3,
min_writer_version: 7,
writer_features: Some(hashset! {WriterFeatures::TimestampWithoutTimezone}),
writer_features: Some(
hashset! {WriterFeatures::TimestampWithoutTimezone, WriterFeatures::Invariants},
),
reader_features: Some(hashset! {ReaderFeatures::TimestampWithoutTimezone}),
}
} else {
Expand Down Expand Up @@ -630,4 +632,43 @@ mod tests {
.clone();
assert_eq!(String::from("value"), value);
}

#[tokio::test]
async fn test_invariants_when_timestampntz_exists() {
use crate::kernel::*;

let columns: Vec<StructField> = vec![
StructField::new("id", DataType::Primitive(PrimitiveType::Integer), false),
StructField::new(
"sunrise",
DataType::Primitive(PrimitiveType::TimestampNtz),
false,
),
];

let table = CreateBuilder::new()
.with_location("memory://")
.with_columns(columns)
.await
.expect("Failed to create the table");

// Ensure that the table can be created properly with the timestmapntz
assert_eq!(table.version(), 0);
let protocol = table.protocol().expect("Failed to load the protocol");

assert_eq!(protocol.min_reader_version, 3);
assert_eq!(protocol.min_writer_version, 7);

let writer_features = protocol.writer_features.as_ref().unwrap();
assert!(
writer_features.contains(&WriterFeatures::TimestampWithoutTimezone),
"The writer features must have TimestampeWithoutTimezone at writer:v7: {:#?}",
writer_features
);
assert!(
writer_features.contains(&WriterFeatures::Invariants),
"The writer features must have Invariants at writer:v7: {:#?}",
writer_features
);
}
}
2 changes: 1 addition & 1 deletion python/tests/test_alter.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ def test_add_timestamp_ntz_column(tmp_path: pathlib.Path, sample_table: pa.Table
assert new_protocol.min_reader_version == 3
assert new_protocol.min_writer_version == 7
assert new_protocol.reader_features == ["timestampNtz"]
assert new_protocol.writer_features == ["timestampNtz"]
assert set(new_protocol.writer_features) == set(["timestampNtz", "invariants"])


features = [
Expand Down
9 changes: 8 additions & 1 deletion python/tests/test_checkpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,14 @@ def test_features_maintained_after_checkpoint(tmp_path: pathlib.Path):
protocol_after_checkpoint = dt.protocol()

assert protocol_after_checkpoint.reader_features == ["timestampNtz"]
assert current_protocol == protocol_after_checkpoint
assert (
protocol_after_checkpoint.min_reader_version
== current_protocol.min_reader_version
)
assert (
protocol_after_checkpoint.min_writer_version
== current_protocol.min_writer_version
)


def test_features_null_on_below_v3_v7(tmp_path: pathlib.Path):
Expand Down
6 changes: 5 additions & 1 deletion python/tests/test_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ def test_create_roundtrip_metadata(tmp_path: pathlib.Path, sample_data: pa.Table
}
assert dt.history()[0]["userName"] == "John Doe"

assert {*dt.protocol().writer_features} == {"appendOnly", "timestampNtz"} # type: ignore
assert {*dt.protocol().writer_features} == {
"appendOnly",
"timestampNtz",
"invariants",
} # type: ignore


def test_create_modes(tmp_path: pathlib.Path, sample_data: pa.Table):
Expand Down
2 changes: 1 addition & 1 deletion python/tests/test_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -1760,7 +1760,7 @@ def test_write_timestamp_ntz_nested(tmp_path: pathlib.Path, array: pa.array):
assert protocol.min_reader_version == 3
assert protocol.min_writer_version == 7
assert protocol.reader_features == ["timestampNtz"]
assert protocol.writer_features == ["timestampNtz"]
assert set(protocol.writer_features) == set(["invariants", "timestampNtz"])


def test_write_timestamp_ntz_on_table_with_features_not_enabled(tmp_path: pathlib.Path):
Expand Down
Loading