Skip to content
Open
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
3 changes: 3 additions & 0 deletions protos/table.proto
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ message Manifest {
// MemWAL SSTables still hold. Writers must refuse it too: one that does not
// maintain index_catchup can change an index without withdrawing the
// position recorded for it. Setting it is one-way.
// * 1 << 8: reserved for datasets that may reference recognized V2 data files
// with different exact versions. Implementations that do not support the
// per-file exact-version contract must treat this bit as unknown.
uint64 reader_feature_flags = 9;

// Feature flags for writers.
Expand Down
25 changes: 19 additions & 6 deletions python/python/tests/test_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -5301,7 +5301,12 @@ def _write_overlay_file(
)


def test_data_overlay_dense(tmp_path: Path):
@pytest.fixture
def enable_unstable_data_overlay_files(monkeypatch):
monkeypatch.setenv("LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES", "1")


def test_data_overlay_dense(tmp_path: Path, enable_unstable_data_overlay_files):
base_dir = tmp_path / "test"
table = pa.table(
{
Expand Down Expand Up @@ -5333,7 +5338,7 @@ def test_data_overlay_dense(tmp_path: Path):
assert result.column("id").to_pylist() == list(range(10))


def test_data_overlay_newest_wins(tmp_path: Path):
def test_data_overlay_newest_wins(tmp_path: Path, enable_unstable_data_overlay_files):
base_dir = tmp_path / "test"
table = pa.table(
{
Expand Down Expand Up @@ -5387,7 +5392,9 @@ def test_data_overlay_newest_wins(tmp_path: Path):
assert val[4] == 444 # only the older overlay covers offset 4


def test_data_overlay_sparse_per_field(tmp_path: Path):
def test_data_overlay_sparse_per_field(
tmp_path: Path, enable_unstable_data_overlay_files
):
base_dir = tmp_path / "test"
table = pa.table(
{
Expand Down Expand Up @@ -5427,7 +5434,9 @@ def test_data_overlay_sparse_per_field(tmp_path: Path):
assert result.column("val").to_pylist()[2] == 20


def test_data_overlay_round_trips_through_fragment_metadata(tmp_path: Path):
def test_data_overlay_round_trips_through_fragment_metadata(
tmp_path: Path, enable_unstable_data_overlay_files
):
import json

base_dir = tmp_path / "test"
Expand Down Expand Up @@ -5480,7 +5489,9 @@ def test_data_overlay_round_trips_through_fragment_metadata(tmp_path: Path):
assert result.column("id").to_pylist() == list(range(10))


def test_data_overlay_rejects_invalid_offsets(tmp_path: Path):
def test_data_overlay_rejects_invalid_offsets(
tmp_path: Path, enable_unstable_data_overlay_files
):
base_dir = tmp_path / "test"
table = pa.table({"val": pa.array([0, 1, 2], pa.int32())})
dataset = lance.write_dataset(table, base_dir)
Expand Down Expand Up @@ -5522,7 +5533,9 @@ def test_data_overlay_rejects_invalid_offsets(tmp_path: Path):
[[1, 1]], # sparse, duplicate
],
)
def test_data_overlay_rejects_unsorted_offsets(tmp_path: Path, offsets):
def test_data_overlay_rejects_unsorted_offsets(
tmp_path: Path, offsets, enable_unstable_data_overlay_files
):
# Offsets map positionally to value rows in data_file. A RoaringBitmap would
# silently reorder/dedup them, so a non-ascending list must be rejected up
# front rather than corrupting the row mapping.
Expand Down
3 changes: 2 additions & 1 deletion rust/lance-namespace-impls/src/dir/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ use lance_namespace::models::{
TableExistsRequest,
};
use lance_namespace::schema::arrow_schema_to_json;
use lance_table::feature_flags::apply_feature_flags;
use lance_table::feature_flags::{apply_feature_flags, ensure_can_write_manifest};
use lance_table::format::{Fragment, IndexMetadata, Manifest};
use lance_table::io::commit::{
CommitError, CommitHandler, commit_handler_from_url, write_manifest_file_to_path,
Expand Down Expand Up @@ -1839,6 +1839,7 @@ impl ManifestNamespace {
indices: Option<Vec<IndexMetadata>>,
transaction: Transaction,
) -> std::result::Result<(), CommitError> {
ensure_can_write_manifest(manifest).map_err(CommitError::from)?;
apply_feature_flags(manifest, false, false).map_err(CommitError::from)?;
let timestamp_nanos = SystemTime::now()
.duration_since(UNIX_EPOCH)
Expand Down
150 changes: 89 additions & 61 deletions rust/lance-table/src/feature_flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,18 @@ pub const FLAG_UNSTABLE_DATA_OVERLAY_FILES: u64 = 64;
/// invalidating the catch-up position recorded for that index, leaving a stale
/// position behind. Both must refuse the table.
pub const FLAG_MEM_WAL_INDEX_CATCHUP: u64 = 128;
/// Reserved for datasets that reference recognized V2 data files with
/// different exact versions.
pub const FLAG_MIXED_DATA_FILE_VERSIONS: u64 = 256;
/// The first bit that is unknown as a feature flag
pub const FLAG_UNKNOWN: u64 = 256;
pub const FLAG_UNKNOWN: u64 = FLAG_MIXED_DATA_FILE_VERSIONS;

// This build only understands flags below the unknown boundary, so a bit
// allocated at or above it would be refused by the very readers meant to use it.
const _: () = assert!(FLAG_MEM_WAL_INDEX_CATCHUP < FLAG_UNKNOWN);
const _: () = assert!(FLAG_MIXED_DATA_FILE_VERSIONS == FLAG_UNKNOWN);

pub(crate) const STICKY_PAIRED_FLAGS: u64 = FLAG_MEM_WAL_INDEX_CATCHUP;

/// Environment variable that opts a release build into reading and writing data
/// overlay files before the feature is generally released.
Expand All @@ -63,18 +69,7 @@ pub fn apply_feature_flags(
// would clear it immediately before the write, so an activated table would
// report success and stay legacy.
//
// Only a consistent state carries: one bit set is neither mode, and
// `inherit_mem_wal_index_catchup` refuses it at the boundary where a
// manifest is derived from another. Reaching here half-set means the
// manifest was already written that way, so leave it for the reader check
// rather than silently completing it.
let mem_wal_index_catchup = if manifest.reader_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP != 0
&& manifest.writer_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP != 0
{
FLAG_MEM_WAL_INDEX_CATCHUP
} else {
0
};
let sticky_paired_flags = validated_sticky_paired_flags(manifest)?;

// Reset flags
manifest.reader_feature_flags = 0;
Expand Down Expand Up @@ -134,38 +129,27 @@ pub fn apply_feature_flags(
manifest.writer_feature_flags |= FLAG_DISABLE_TRANSACTION_FILE;
}

manifest.reader_feature_flags |= mem_wal_index_catchup;
manifest.writer_feature_flags |= mem_wal_index_catchup;
manifest.reader_feature_flags |= sticky_paired_flags;
manifest.writer_feature_flags |= sticky_paired_flags;

Ok(())
}

/// Carry [`FLAG_MEM_WAL_INDEX_CATCHUP`] from the manifest a new one is derived
/// Carry sticky paired capabilities from the manifest a new one is derived
/// from.
///
/// [`apply_feature_flags`] carries this bit across its own reset, but it only
/// ever sees one manifest. It cannot help where a *new* manifest is derived from
/// an existing one -- `Manifest::new_from_previous` and `shallow_clone` both
/// zero the feature words -- because the destination starts with nothing to
/// carry. That transition is this function's job.
/// [`apply_feature_flags`] carries these bits across its own reset, but it only
/// ever sees one manifest. Constructors preserve these flags, and this helper
/// also validates that the source is not half-set before a derived manifest is
/// committed.
///
/// A half-set state is refused rather than normalized: one bit set means a
/// legacy reader or a legacy writer is still permitted, which is neither mode.
pub fn inherit_mem_wal_index_catchup(destination: &mut Manifest, source: &Manifest) -> Result<()> {
let reader = source.reader_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP != 0;
let writer = source.writer_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP != 0;
match (reader, writer) {
(false, false) => Ok(()),
(true, true) => {
destination.reader_feature_flags |= FLAG_MEM_WAL_INDEX_CATCHUP;
destination.writer_feature_flags |= FLAG_MEM_WAL_INDEX_CATCHUP;
Ok(())
}
_ => Err(Error::invalid_input(
"Manifest has only one of the MemWAL index-catchup reader and writer \
feature bits set, so its catch-up semantics are undefined",
)),
}
pub fn inherit_sticky_feature_flags(destination: &mut Manifest, source: &Manifest) -> Result<()> {
let sticky_flags = validated_sticky_paired_flags(source)?;
destination.reader_feature_flags |= sticky_flags;
destination.writer_feature_flags |= sticky_flags;
Ok(())
}

/// Whether this build understands data overlay files: always in debug builds,
Expand Down Expand Up @@ -208,28 +192,68 @@ pub fn can_write_dataset(writer_flags: u64) -> bool {
writer_flags & !supported_flags() == 0
}

/// Refuse reads from manifests whose required reader features this build does
/// not support or whose paired capabilities are inconsistent.
pub fn ensure_can_read_manifest(manifest: &Manifest) -> Result<()> {
validate_paired_feature_flags(manifest)?;
if !can_read_dataset(manifest.reader_feature_flags) {
return Err(Error::not_supported_source(
format!(
"This dataset cannot be read by this version of Lance. Please upgrade \
Lance to read this dataset. Flags: {}",
manifest.reader_feature_flags
)
.into(),
));
}
Ok(())
}

/// Refuse writes to manifests whose required writer features this build does
/// not support or whose paired capabilities are inconsistent.
pub fn ensure_can_write_manifest(manifest: &Manifest) -> Result<()> {
validate_paired_feature_flags(manifest)?;
if !can_write_dataset(manifest.writer_feature_flags) {
return Err(Error::not_supported_source(
format!(
"This dataset cannot be written by this version of Lance. Please upgrade \
Lance to write this dataset. Flags: {}",
manifest.writer_feature_flags
)
.into(),
));
}
Ok(())
}

pub fn has_deprecated_v2_feature_flag(writer_flags: u64) -> bool {
writer_flags & FLAG_USE_V2_FORMAT_DEPRECATED != 0
}

/// Refuse a manifest whose MemWAL index-catchup bits disagree.
/// Refuse a manifest whose paired reader and writer capability bits disagree.
///
/// One word set and the other not is neither mode: it would let a legacy reader
/// or a legacy writer through on a table where the other half is enforcing. The
/// commit path refuses to *produce* this, so seeing it on read means the
/// manifest was written by something that did not.
pub fn validate_mem_wal_index_catchup_flags(manifest: &Manifest) -> Result<()> {
pub fn validate_paired_feature_flags(manifest: &Manifest) -> Result<()> {
let reader = manifest.reader_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP != 0;
let writer = manifest.writer_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP != 0;
if reader != writer {
return Err(Error::invalid_input(
"Manifest has only one of the MemWAL index-catchup reader and writer \
feature bits set, so its catch-up semantics are undefined",
return Err(Error::corrupt_file_named(
"manifest",
"Manifest has only one of the MemWAL index-catchup reader and writer feature bits set, \
so its semantics are undefined",
));
}
Ok(())
}

fn validated_sticky_paired_flags(manifest: &Manifest) -> Result<u64> {
validate_paired_feature_flags(manifest)?;
Ok(manifest.reader_feature_flags & STICKY_PAIRED_FLAGS)
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -395,14 +419,14 @@ mod tests {
/// it, or a later transaction would silently downgrade the table to legacy
/// semantics and a reader would treat missing coverage as complete.
#[test]
fn inheriting_carries_the_mem_wal_bit_from_the_source() {
fn inheriting_carries_sticky_paired_bits_from_the_source() {
let mut source = empty_manifest();
source.reader_feature_flags = FLAG_MEM_WAL_INDEX_CATCHUP;
source.writer_feature_flags = FLAG_MEM_WAL_INDEX_CATCHUP;
// What `Manifest::new_from_previous` hands us: both words zeroed.
// A fresh destination models any derived manifest before inheritance.
let mut destination = empty_manifest();

inherit_mem_wal_index_catchup(&mut destination, &source).unwrap();
inherit_sticky_feature_flags(&mut destination, &source).unwrap();

assert_ne!(
destination.reader_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP,
Expand All @@ -425,14 +449,14 @@ mod tests {
source.writer_feature_flags = writer;
let mut destination = empty_manifest();

let err = inherit_mem_wal_index_catchup(&mut destination, &source).unwrap_err();
let err = inherit_sticky_feature_flags(&mut destination, &source).unwrap_err();

assert!(err.to_string().contains("only one of"), "{err}");
}
}

#[test]
fn apply_feature_flags_carries_the_mem_wal_bit_across_its_reset() {
fn apply_feature_flags_carries_sticky_paired_bits_across_its_reset() {
// It runs twice per commit -- `build_manifest` and `write_manifest_file`
// -- so dropping the bit here would clear it immediately before the
// write, and an activated table would report success and stay legacy.
Expand All @@ -453,21 +477,25 @@ mod tests {
}

#[test]
fn apply_feature_flags_drops_a_half_set_mem_wal_bit() {
// Neither mode, so leave it for the reader check rather than completing it.
fn apply_feature_flags_rejects_half_set_sticky_bits() {
let mut manifest = empty_manifest();
manifest.reader_feature_flags = FLAG_MEM_WAL_INDEX_CATCHUP;

apply_feature_flags(&mut manifest, false, false).unwrap();
let err = apply_feature_flags(&mut manifest, false, false).unwrap_err();

assert_eq!(
manifest.reader_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP,
0
);
assert_eq!(
manifest.writer_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP,
0
);
assert!(matches!(err, Error::CorruptFile { .. }));
assert!(err.to_string().contains("only one of"), "{err}");
}

#[test]
fn writer_gate_rejects_reserved_mixed_capability() {
let mut manifest = empty_manifest();
manifest.reader_feature_flags = FLAG_MIXED_DATA_FILE_VERSIONS;
manifest.writer_feature_flags = FLAG_MIXED_DATA_FILE_VERSIONS;

let err = ensure_can_write_manifest(&manifest).unwrap_err();
assert!(matches!(err, Error::NotSupported { .. }));
assert!(err.to_string().contains("cannot be written"), "{err}");
}

fn empty_manifest() -> Manifest {
Expand All @@ -489,11 +517,11 @@ mod tests {
/// A build that does not know the bit must refuse the table rather than
/// continue with legacy semantics.
#[test]
fn the_mem_wal_bit_is_below_the_unknown_boundary() {
fn mixed_capability_remains_at_the_unknown_boundary() {
assert!(can_read_dataset(FLAG_MEM_WAL_INDEX_CATCHUP));
assert!(can_write_dataset(FLAG_MEM_WAL_INDEX_CATCHUP));
// The next bit up is still unknown, so allocating this one did not
// silently widen what this build claims to understand.
assert!(!can_read_dataset(FLAG_UNKNOWN));
assert!(!can_read_dataset(FLAG_MIXED_DATA_FILE_VERSIONS));
assert!(!can_write_dataset(FLAG_MIXED_DATA_FILE_VERSIONS));
assert_eq!(FLAG_MIXED_DATA_FILE_VERSIONS, FLAG_UNKNOWN);
}
}
10 changes: 5 additions & 5 deletions rust/lance-table/src/format/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::ops::Range;
use std::sync::Arc;

use super::Fragment;
use crate::feature_flags::FLAG_MEM_WAL_INDEX_CATCHUP;
use crate::feature_flags::STICKY_PAIRED_FLAGS;
use crate::feature_flags::{FLAG_STABLE_ROW_IDS, has_deprecated_v2_feature_flag};
use crate::format::fragment::DataFileFieldInterner;
use crate::format::pb;
Expand Down Expand Up @@ -219,8 +219,8 @@ impl Manifest {
index_section: None, // Caller should update index if they want to keep them.
timestamp_nanos: 0, // This will be set on commit
tag: None,
reader_feature_flags: 0, // These will be set on commit
writer_feature_flags: 0, // These will be set on commit
reader_feature_flags: previous.reader_feature_flags & STICKY_PAIRED_FLAGS,
writer_feature_flags: previous.writer_feature_flags & STICKY_PAIRED_FLAGS,
max_fragment_id: previous.max_fragment_id,
transaction_file: None,
transaction_section: None,
Expand Down Expand Up @@ -279,8 +279,8 @@ impl Manifest {
// Not derivable from the manifest, so it would be lost like any
// other zeroed word -- and a clone of a table that requires index
// catch-up would silently come back as legacy.
reader_feature_flags: self.reader_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP,
writer_feature_flags: self.writer_feature_flags & FLAG_MEM_WAL_INDEX_CATCHUP,
reader_feature_flags: self.reader_feature_flags & STICKY_PAIRED_FLAGS,
writer_feature_flags: self.writer_feature_flags & STICKY_PAIRED_FLAGS,
max_fragment_id: self.max_fragment_id,
transaction_file: Some(transaction_file),
transaction_section: None,
Expand Down
Loading
Loading