Repository navigation
Stack/kernel partition timezone cast - #5
Open
damn-am-star wants to merge 522 commits into
Open
damn-am-star wants to merge 522 commits into
damn-am-star wants to merge 522 commits into
Conversation
## What changes are proposed in this pull request? The java kernel's DeltaHistoryManager has a public method [getEarliestDeltaFile](https://github.com/delta-io/delta/blob/master/kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java#L456) that allows client to get the earliest commit's json version available in the tables which rust currently lacks. This PR add an implementation of the `getEarliestDeltaFile` to the kernel rust. In the future, a public API will use this helper function to retrieve the earliest delta file. ## How was this change tested? New unit test was added.
…ta-io#2663) ## What changes are proposed in this pull request? The `timestamp-to-version` search now plumbs the matched commit's timestamp through alongside its version via a new public Commit struct. `latest_version_as_of` and `first_version_after` now return `Commit` instead of a bare `Version`. For the non-ICT (file modification) commits, the returned timestamp is the monotonized timestamp (not the raw file modification time), matching Delta Spark's DeltaHistoryManager behavior. For ICT commits it is the in-commit timestamp. <!-- **Uncomment** this section if there are any changes affecting public APIs. Else, **delete** this section. ### This PR affects the following public APIs If there are breaking changes, please ensure the `breaking-changes` label gets added by CI, and describe why the changes are needed. Note that _new_ public APIs are not considered breaking. --> ## How was this change tested? Unit tests is updated to assert both version and timestamp of the returned Commit
delta-io#2522) ## 🥞 Stacked PR Use this [link](https://github.com/delta-io/delta-kernel-rs/pull/2522/files) to review incremental changes. - [stack/test-table-builder-last-checkpoint-hint](delta-io#2521) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2521/files)] [MERGED] - [**stack/test-table-builder-crc**](delta-io#2522) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2522/files)] --------- ## What changes are proposed in this pull request? Adds a crc_at axis to LogState in TestTableBuilder. Writes a CRC file at a given version via kernel's Snapshot::write_checksum. Also adds four CRC rows to the canonical default_sweep: 1. CRC-at-latest 2. stale CRC at mid 3. checkpoint + CRC at latest 4. checkpoint at latest+ stale crc ## How was this change tested? unit tests + cross-product integration sweep
## What changes are proposed in this pull request?
`ColumnName::new` takes an `impl FromIterator` arg, which requires a
complicated type bound:
```rust
fn new<A>(arg: impl IntoIterator<Item = A>) -> Self
where
Self: FromIterator<A>
{
arg.into_iter().collect()
}
```
The same type bound messiness "poisons" any function that wants to
forward args to `ColumnName::new`.
Observing that the same problem exists with `From`, and is solved by the
`Into` trait, define a new `CollectInto` trait that is the analogue of
`Into` but for `FromIterator`. It is defined automatically for all
eligible types, and allows the much simpler type bound:
```rust
fn new(arg: impl CollectInto<Self>) -> Self {
arg.collect_into()
}
```
### This PR affects the following public APIs
The switch to `CollectInto` is a breaking change for `ColumnName::new`,
`Predicate::column`, `Expression::column`, and
`ExpressionStructPatch::new_nested`. Even tho those methods accept all
the same types as before (thanks to the trait's blanket impl), their
signatures no longer have a template arg, which breaks turbofish call
sites (see the updated unit test).
Note:
* This is a potential alternative to
delta-io#2683
* The other achieves a similar result by changing `ColumnName::new` to
take `impl Into<ColumnName>`
* This PR has the disadvantage that it requires a new type
* The other PR has the disadvantage that it narrows the set of types
that `ColumnName::new` can accept, because both `Into` and `From`
already have blanket impl that block us from adding a new blanket impl
for all T: IntoIterator.
## How was this change tested?
Existing unit tests exercise these code paths.
…ilerplate (delta-io#2700) ## What changes are proposed in this pull request? Many (many) call sites that create new data types are doing it the hard way. Update them to use helper constructors that reduce boilerplate. Also teach `ArrayType::new` to accept `impl Into<DataType>` (and update callers to leverage it). As an extreme (but common) example, code like this: ```rust let array_ty = DataType::Array(Box::new(ArrayType::new( DataType::Struct(Box::new(inner_field_schema)), false, )))); ``` Can simplify down to just: ```rust let array_ty = DataType::from(ArrayType::new(inner_field_schema, false)); ``` ### This PR affects the following public APIs The new `ArrayType::new` signature can cause compiler errors for callers who relied on un-annotated `.into()` to convert args into data types. Fix: remove those now-redundant `.into()` calls. ## How was this change tested? Lots and lots of unit tests updated.
…io#2697) ## What changes are proposed in this pull request? These actions were broken for runs that were not `pull_request` triggered, either because the logic to try and pull the info was wrong, or just missing. This PR adds code to check why the jobs where triggered and to fetch the title/body in the appropriate way given the trigger. There are code comments that should explain the changes. Also this renames pr-validator.yml to pr-title-validator.yml as that's really what it is. ## How was this change tested? I've ensured the PR based ones still work as expected. We have to merge to test the other ones, but those are already broken so at least this won't make things _worse_ :) I've also run the scripts locally with crafted environment variables to try and ensure as much as possible that things kinda work.
## What changes are proposed in this pull request?
Adds a plan intermediate representation (IR) under the
`declarative-plans` feature: a 9-variant `NodeKind` enum with
per-variant payload structs, a `Plan` container holding a
`Vec<PlanNode>` wired into a DAG by `RefId` inputs and outputs, and a
`ResultPlan` pairing a `Plan` with the terminal `RefId` whose rows the
engine streams to the caller.
- `kernel/src/plans/ir/plan.rs`: `Plan`, `PlanNode`, `RefId`,
`ResultPlan`.
- `kernel/src/plans/ir/nodes.rs`: `NodeKind` (source / unary / binary /
n-ary operator groups) and payload structs (`ScanParquetNode`,
`ScanJsonNode`, `ValuesNode`, `ProjectNode`, `FilterNode`, `LoadNode`,
`MaxByVersionNode`, `EquiJoinNode`, `UnionAllNode`), plus helper types
(`FileType`, `LoadColumnFileMeta`, `JoinKind`).
- `kernel/src/plans/ir/operation.rs`: upstream's `Operation` /
`IoOperation` / `QueryPlan` / `QueryPlanNode`, relocated from `ir.rs`
into the new directory.
- `kernel/Cargo.toml`: `declarative-plans` now enables `internal-api`
(the IR uses internal-api-gated `ToSchema`) and `arrow` (downstream
consumers depend on Arrow types).
- `kernel/src/engine/mod.rs`: tightens the `pub use
arrow_utils::{parse_json, to_json_bytes}` gate from `internal-api` alone
to `internal-api + arrow-expression`. The pre-existing gate failed to
compile when `internal-api` was on but `arrow-expression` was off.
## How was this change tested?
`cargo build`, `cargo clippy --tests`, and `cargo doc` all pass with
`--features declarative-plans`. The IR is pure data with no behavior to
unit-test until the bridge to `PlanExecutor` lands in the follow-up PR
(stack/bridge).
…lta-io#2666) ## What changes are proposed in this pull request? Replaces the four ad-hoc stats setters on `ScanBuilder` with a single `with_stats(StatsOptions)` method. The struct exposes two orthogonal axes that the old API muddled: JSON stats (`add.stats`) and struct stats (`add.stats_parsed`). ```rust .with_stats(StatsOptions::default()) // JSON only (today's default) .with_stats(StatsOptions::all_struct()) // struct only, no ToJson cost .with_stats(StatsOptions::struct_columns(cols)) // struct projected, no JSON .with_stats(StatsOptions::all()) // both .with_stats(StatsOptions::none()) // neither, AND disables internal data skipping ``` `StatsOptions::none()` is the only constructor that stops kernel from reading stats at all (no output and no internal skipping). The other four keep predicate-based skipping running even when stats output is suppressed. ### This PR affects the following public APIs Removed: `include_all_stats_columns`, `with_stats_columns`, `with_skip_stats`, `StatsOutputMode`. Added: `with_stats(StatsOptions)`, `StatsOptions`, `StructStats`. ## How was this change tested? Integration tests in `tests/integration/read.rs` parameterized over the four `StatsOptions` shapes x five checkpoint variants. Unit test directly asserts the add transform contains zero `ToJson` nodes when `json=false`. `cargo nextest`, `cargo clippy`, `cargo doc` all clean.
## What changes are proposed in this pull request? This PR implements FFI structs for `PlanResult` and associated iterator result types. Each PlanResult variant has an associated "C iter" struct and an "FFI adapter" struct. For example for `PlanResult::Data`, we introduce: - `CEngineDataIterator` - a raw repr(c) struct that holds a `state` + `free` callback fn that can be used to implement an iterator over FFI. - `FfiEngineDataIter` - wraps a `CEngineDataIterator` and implements the Rust iterator trait. A few important callouts regarding memory management: - We use a `CPlanResultWrapper` to provide a single `free` callback that will release all resources related to the plan result. This let's Java use a single shared arena for the entire plan result isntead of providing separate `free` fn for each nested item. - Each iterator type has slightly different semantics: - For EngineData, we expect to use ArrowEngineData which will internally release memory for each batch (on drop of each item). - For Bytes, we use FFI_ArrowArray to pass bytes across FFI, which then also gives us the same memory management as ArrowEngineData (on drop of each item) - For FileMeta, we rely on the PlanResult cleanup to drop all FileMeta within the iterator at once (on drop of the iterator) ## How was this change tested? Unit tests
## What changes are proposed in this pull request? Follow-up to delta-io#2623. Same decorator pattern, now applied to JSON and Parquet. - New `MeteredJsonHandler` and `MeteredParquetHandler` wrap any handler and emit `JsonReadCompleted` / `ParquetReadCompleted` once per `read_*_files` call. Other trait methods pass through. - `MeteredDeltaEngine` now wraps all three handlers (storage + JSON + Parquet), each guarded against double-wrap. - `DefaultJsonHandler` and `DefaultParquetHandler` no longer self-meter. `DefaultEngine` wraps them at construction, so default-engine users see no behavior change. A custom-engine consumer gets all five events with`Arc::new(MeteredDeltaEngine::new(inner))`. ## How was this change tested? New unit tests in `metered_json.rs` and `metered_parquet.rs`
## What changes are proposed in this pull request? Bridges `Operation::QueryPlan` onto the SSA-DAG IR from stack/c4. The variant now carries a `ResultPlan` (a `Plan` DAG plus its terminal `RefId`) instead of the single-node `QueryPlanNode` introduced in delta-io#2621. - `kernel/src/plans/ir/operation.rs`: `Operation::QueryPlan(QueryPlan)` becomes `Operation::QueryPlan(ResultPlan)`. The `pub type QueryPlan = QueryPlanNode` alias and the `QueryPlanNode` enum are removed; their `ScanJson` / `ScanParquet` variants are subsumed by `NodeKind::ScanJson` / `NodeKind::ScanParquet`. - `kernel/src/plans/query_builder.rs`: `QueryPlanBuilder::scan_json` / `scan_parquet` keep their public signatures but now construct a single-node `Plan` wrapped in a `ResultPlan { result: RefId(0) }`. - `kernel/src/engine/sync/plan.rs::execute_query`: calls `ResultPlan::into_single_node_plan` and dispatches on `NodeKind`. - `kernel/src/lib.rs`, `kernel/src/plans/{mod.rs, ir/mod.rs}`: drop `QueryPlan` and `QueryPlanNode` from the public re-exports. The `PlanBasedJsonHandler` / `PlanBasedParquetHandler` call sites are unchanged — only the builder's internals and the executor's match arms moved. ## Breaking change Under the `declarative-plans` feature, the `Operation::QueryPlan` variant's payload changes from `QueryPlanNode` to `ResultPlan`, the `QueryPlan` type alias and `QueryPlanNode` enum are removed, and `QueryPlanBuilder::build()` now returns `DeltaResult<ResultPlan>` instead of `DeltaResult<QueryPlan>`. Implementations of `PlanExecutor` and anyone matching on `Operation::QueryPlan` must update accordingly. ## How was this change tested? `cargo build --features declarative-plans`, `cargo clippy --features declarative-plans --tests -- -D warnings`, and `cargo nextest run --features declarative-plans plans engine::plans engine::sync::plan` all pass — 10/10 plans-tests green, including the `PlanBased*Handler` round-trips that now go through the SSA-DAG IR.
## What changes are proposed in this pull request? Previously we've had `on: [push, pull_request, merge_group]` in our CI workflows. This allowed for duplicate CI runs in two cases: - Case 1: PR added to merge gueue. This **pushes** to a new temp branch. Thus the **merge_group** and the **push** events were triggered, and 2 runs were executed - Case 2: A branch in this repo (**not in a fork**) was pushed to. Again, **pull_request** and **push** events were triggered. This PR changes this so that we only execute push against `main` and `release` branches. Evidence: Every merge-queue entry produced a matching `merge_group` + `push` pair of `build` runs on the same `gh-readonly-queue/*` temp branch and the same commit, created ~1 second apart: | PR | temp-branch commit | `merge_group` run | `push` run (redundant) | |----|--------------------|-------------------|------------------------| | delta-io#2522 | `a4ff55919c` | [26986967449](https://github.com/delta-io/delta-kernel-rs/actions/runs/26986967449) | [26986967842](https://github.com/delta-io/delta-kernel-rs/actions/runs/26986967842) | | delta-io#2613 | `0c0414abbb` | [27033496212](https://github.com/delta-io/delta-kernel-rs/actions/runs/27033496212) | [27033496903](https://github.com/delta-io/delta-kernel-rs/actions/runs/27033496903) | | delta-io#2663 | `c67799f4d0` | [26986255295](https://github.com/delta-io/delta-kernel-rs/actions/runs/26986255295) | [26986255687](https://github.com/delta-io/delta-kernel-rs/actions/runs/26986255687) | | delta-io#2666 | `3638ba8433` | [27035061234](https://github.com/delta-io/delta-kernel-rs/actions/runs/27035061234) | [27035061155](https://github.com/delta-io/delta-kernel-rs/actions/runs/27035061155) | | delta-io#2680 | `b23ec52a24` | [27035546413](https://github.com/delta-io/delta-kernel-rs/actions/runs/27035546413) | [27035546610](https://github.com/delta-io/delta-kernel-rs/actions/runs/27035546610) | | delta-io#2694 | `0ee0f738b7` | [27036278360](https://github.com/delta-io/delta-kernel-rs/actions/runs/27036278360) | [27036279320](https://github.com/delta-io/delta-kernel-rs/actions/runs/27036279320) | | delta-io#2697 | `ae7a2b8c6e` | [27032663202](https://github.com/delta-io/delta-kernel-rs/actions/runs/27032663202) | [27032663624](https://github.com/delta-io/delta-kernel-rs/actions/runs/27032663624) | | delta-io#2699 | `a4871e2836` | [27028497624](https://github.com/delta-io/delta-kernel-rs/actions/runs/27028497624) | [27028497786](https://github.com/delta-io/delta-kernel-rs/actions/runs/27028497786) | | delta-io#2700 | `3ca5f50a02` | [27031189908](https://github.com/delta-io/delta-kernel-rs/actions/runs/27031189908) | [27031189924](https://github.com/delta-io/delta-kernel-rs/actions/runs/27031189924) | Across the last 500 `build` runs this pairing was exact: 38 `merge_group` runs and 38 redundant `push` runs on `gh-readonly-queue/*` branches. ## How was this change tested? CI.
## What changes are proposed in this pull request? Extracts `kernel/src/engine/default/` into a new `delta_kernel_default_engine` crate and rewires workspace consumers. Split into 5 commits: 1. File moves, new crate manifest, drop kernel-side `default-engine-*` feature flags. 2. `#[internal_api]` adds, stats verifier tests relocated, handler contract helpers moved to `test_utils`. 3. Cargo.toml + import + doc path churn across consumers. 4. User guide + mdbook test target. 5. Address Ken's review comments: new `default_engine_test_utils` sub-crate, restored commented-out contract tests with TODOs, visibility refactor on `StatsColumnVerifier`. ### Circular dependency situation There is a dev-cycle: `kernel (dev-dep) -> test_utils -> default_engine -> kernel`. It exists on purpose because kernel's integration tests test kernel's public API and want to live in `kernel/tests/`. `test_utils` re-exports `delta_kernel_default_engine` so those tests do not need to rewire imports. Consequences: - Handler contract tests no longer run against `SyncEngine`. They still run against `DefaultEngine`. (The cycle through `test_utils` makes the trait types non-unifiable when called from kernel's own test code.) - Kernel's test build now links arrow (via `test_utils -> default_engine`). Production builds of kernel do not. ### This PR affects the following public APIs `delta_kernel::engine::default::*` is removed. Consumers depend on `delta_kernel_default_engine` directly and pick a TLS backend (`rustls` or `native-tls`). ## How was this change tested? `cargo nextest run --workspace --all-features` passes locally.
## What changes are proposed in this pull request? Set `stats.tightBound` as false, for dv updates. Resolves delta-io#2664 <!-- **Uncomment** this section if there are any changes affecting public APIs. Else, **delete** this section. ### This PR affects the following public APIs If there are breaking changes, please ensure the `breaking-changes` label gets added by CI, and describe why the changes are needed. Note that _new_ public APIs are not considered breaking. --> ## How was this change tested? Test tightBound = true/false/unset, assert `tightBound == false` after dv update and checkpoint
## What changes are proposed in this pull request? Three CI fixes: - Add `concurrency` to several jobs: pushing to your PR now cancels the previous run (no one looks at it anyways). This is what delta-rs and datafusion do. - Run with `debuginfo` off (debugger source-line/variable maps, type layouts). Test output, logs, and panics are unaffected ## How was this change tested? CI.
) ## What changes are proposed in this pull request? The coverage job has been failing at the Codecov upload step ([example link](https://github.com/delta-io/delta-kernel-rs/actions/runs/27137413595/job/80109392728?pr=2665)). The version of codecov-action we pin (v5.5.3) verifies the downloaded CLI against a GPG key it pulls from `keybase.io/codecovsecurity`, and that account now just returns `SELF-SIGNED PUBLIC KEY NOT FOUND`. So verification dies with `Can't check signature: No public key`. Codecov moved their signing key to a different keybase account and only pointed the action at it in a later release. Bumping to v7.0.0 (the same version delta-rs runs) picks up the wrapper that reads from the working account. GPG verification stays on, no `skip_validation` or other workaround. This should also unfreeze our coverage build cache as a side effect: the failing upload made the job fail, which skips Swatinem's cache-save step (`post-if: success()`), so the cache went stale and every coverage run was rebuilding from scratch. ## How was this change tested? CI on this PR.
) ## What changes are proposed in this pull request? This PR connects the existing incremental CRC replay infrastructure into our Snapshot construction workflow. There are some important followups after this PR is merged: - Metrics - Better incremental Snapshot build support. That is: Snapshot at v15 with v15.crc. When we incrementally go to build a newer snapshot at v17 (no new CRC), we should be doing incremental CRC replay against that 15.crc. Out of scope of this current PR. See the CRC tracking issue delta-io#1781 ## How was this change tested? New UTs.
…-builder (delta-io#2649) ## What changes are proposed in this pull request? Addressing delta-io#1766. Adds a new integration test to test nested struct parsing with the proposed schema from the issue. Design choices: - Used functionality from delta-io#1643 to write parsed stats in checkpoints for testing - Left the original test_scan_metadata_with_stats_columns unchanged - moved the `get_column` macro from unit tests to `test_utils` for accessibility from integration tests ## How was this change tested? This change doesn't affect production code. --------- Co-authored-by: Ayush Raj <a.raj@databricks.com>
## What changes are proposed in this pull request? This PR adds an `FfiPlanExecutor` struct and associated FFI functions for creating / free'ing it. `FfiPlanExecutor` implements the `PlanExecutor` trait and enables foreign Engines (e.g Java-based) to implement PlanExecutor through a single callback. We expect that `FfiPlanExecutor` will proto-serialize a given plan `Operation` and pass it to the callback fn as a byte array. However proto-serialization is still left as a TODO for now. Builds on top of delta-io#2680 ## How was this change tested? Unit tests
## What changes are proposed in this pull request? Resolves delta-io#2658 Today _some_ metric events have success and failure variants, but most don't. On error, Kernel would emit (for example) a `LogSegmentLoaded` event with default-constructed values: a false "success" that pollutes downstream metrics. This PR gives every lifecycle operation a flat success/failure pair, so a failure is never reported as a success. ### Heads up for downstream consumers `MetricEvent` matches will need updating. The variant changes: **Renamed** - `LogSegmentLoaded` → `LogSegmentLoadSuccess` - `ProtocolMetadataLoaded` → `ProtocolMetadataLoadSuccess` - `SnapshotCompleted` → `SnapshotBuildSuccess` - `DomainMetadataLoaded` → `DomainMetadataLoadSuccess` - `SetTransactionLoaded` → `SetTransactionLoadSuccess` - `CrcReadCompleted` → `CrcReadSuccess` - `SnapshotFailed` → `SnapshotBuildFailure` **Added (new failure variants)** - `LogSegmentLoadFailure` - `ProtocolMetadataLoadFailure` - `DomainMetadataLoadFailure` - `SetTransactionLoadFailure` - `CrcReadFailure` ## How was this change tested? New unit tests that force each lifecycle operation to fail and assert it emits the failure variant (not a success).
…m` API (delta-io#2712) ## What changes are proposed in this pull request? Allows incremental snapshot builders (given a Snapshot A, try and construct a newer Snapshot B) to be able to take advantage of incremental CRC reconstruction. This covers several cases: - Imagine we had a Snapshot A at v10 with some in-memory CRC - It doesn't matter if there was a 10.crc or a 5.crc and Snapshot A derived v10.crc via incremental CRC replay - Then a connector wants to build a new Snapshot from A - **Case 1**: That connector sees: 11-20.json -> it builds a Snapshot B at v20 with in-memory CRC using the base from A - **Case 2**: That connector sees 11-20.json with 15.crc -> it builders a Snapshot B at v20 with in-memory CRC using the 15.crc base Resolves delta-io#2674. ## Expected Performance Impact Test Setup: Write to a UC managed table always using a previous Snapshot at N-4 as the "build_from" Snapshot, which **did** have a CRC. 100 parquet files per commit. | Phase | incremental (Unlimited) | master (Disabled) | incremental / master | |---|---|---|---| | snapshot build (build_from) | 60.8 ms | 54.5 ms | 1.12x | | txn build (.transaction()) | 0.0136 ms | 78.0 ms | ~5,700x faster | | txn commit (.commit()) | 25.8 ms | 77.4 ms | 0.33x (3x faster) | | per-commit total | ~87 ms | ~210 ms | ~2.4x faster | ## How was this change tested? New UTs.
## What changes are proposed in this pull request? Fixes a flaky failure in the `metrics deadlock test` CI step (e.g. https://github.com/delta-io/delta-kernel-rs/actions/runs/27173481108/job/80217380098?pr=2665). **Problem:** the step pipes the example's output into `grep -q`: ```bash echo "$output" | grep -q 'invalid_field' ``` `grep -q` exits as soon as it matches, closing the pipe. Both matched strings sit near the end of a large (~159 KB) output, so by then `echo` may still have bytes pending and its write fails with `EPIPE`. Under `bash -eo pipefail` (the `shell: bash` default) that broken-pipe exit becomes the pipeline's status and fails the step. It's a race, hence the flakiness. The failing run shows exactly this: ``` shell: /opt/homebrew/bin/bash --noprofile --norc -e -o pipefail {0} ... .../6fbb8cb3-...sh: line 3: echo: write error: Broken pipe ##[error]Process completed with exit code 1. ``` **Fix:** drop the pipe and feed `grep` a here-string. `grep` reads a complete buffer, so matching early can't break a pipe. ```bash grep -q 'invalid_field' <<< "$output" ``` ## How was this change tested? CI.
…ta-io#2705) ## What changes are proposed in this pull request? This change does three things: (1) Allows devs to manually author new DAT cases. Previously `acceptance/build.rs` could only download pinned release tar files to run DAT. Now, when `DELTA_ACCEPTANCE_WORKLOADS_PATH` is set, devs can generate a DAT case using the workload generator in https://github.com/delta-incubator/dat/ to validate against Kernel. (2) bug fix: Kernel accepted the snapshot spec type as `snapshotConstruction`, but the DAT workload generator emits `snapshot`. (3) update `acceptance/README.md` to document the CUJ to write a DAT case and point `kernel-rs` at it. ## How was this change tested? This script was tested locally against https://github.com/delta-incubator/dat.
## What changes are proposed in this pull request? Adds a prost-generated protobuf layer mirroring the kernel's plan, schema, and expression IR, so engines can ship plans across a process or language boundary. The `.proto` files (`schema`, `expressions`, `plan`) mirror the Rust enums one-to-one; `build.rs` runs prost-build only under `declarative-plans`, and `plans/proto/mod.rs` re-exports the generated modules. The kernel produces these messages but does not interpret them. A follow-up PR adds `From` / `TryFrom` conversions between the kernel IR and the proto messages. Tracking: delta-io#2679 weighs this proto wire format against the existing FFI schema/expression visitors as the mechanism for the IR boundary. ## How was this change tested? `cargo build`, `cargo clippy --tests`, and `cargo doc` are clean with `--features declarative-plans`.
## What changes are proposed in this pull request? This PR changes the tables that we benchmark against and includes stats going forward. We also update some formatting such as making the the tests collapsable and adding a benchmark summary and add some emojis to make scanning the benchmark result much easier (🐌 -> 🚧 -> ✅ -> 🚀). ## How was this change tested? Showed that benchmarks can run after pull request. Each scan looks like a regression because the base did not include stats in the scan.
…elta-io#2675) ## What changes are proposed in this pull request? The java kernel's DeltaHistoryManager has a public method [getEarliestRecreatableCommit(](https://github.com/delta-io/delta/blob/master/kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java#L456) that allows client to get the earliest version that the table can be reconstructed which rust currently lacks. This PR add an implementation of the `getEarliestRecreatableCommit` to the kernel rust. In the future, a public API will use this helper function to retrieve the earliest recreatable commit version. ## How was this change tested? New unit test was added to verify the recreatable version is extracted correctly and the error is return as expected.
Please [use this change](https://github.com/delta-io/delta-kernel-rs/pull/2713/changes/4d61ba853e124e4e4f8bc52c7c18cc13566c287a..7e0f180021191d6b5f5bf07f07eb52d81a29662f) to review the PR. ## What changes are proposed in this pull request? This PR added a new public API in history_manager mod, the `get_earliest_commit`. The function allow user to get the earliest available file-system commit in the `_delta_log/` of the table. Furthermore, user can query for the earliest available version (`HistoryCommitType::Published`) or the earliest version that the table can be reconstructed and replay forward from (`HistoryCommitType::Recreatable`) . This PR depend on: delta-io#2675 ## How was this change tested? New integration test was added to verify the public API. The integration test construct a table with 8 commits, checkpoint at version 5 and various log clean up simulation scenario to test the return value based on the `HistoryCommitType` and the log cleanup.
TODO: depending on which is merged first, bring in the table_type metric label from delta-io#2721 ## What changes are proposed in this pull request? Emit `TransactionCommitSuccess` and `TransactionCommitFailure` metric events. ## How was this change tested? New UTs.
…o#2599) ## What changes are proposed in this pull request? Currently we have `ValidateColumnMappings` and `MakePhysical`, they shared a lot of similar logics, this PR unifies them to one struct. Address delta-io#2589 <!-- **Uncomment** this section if there are any changes affecting public APIs. Else, **delete** this section. ### This PR affects the following public APIs If there are breaking changes, please ensure the `breaking-changes` label gets added by CI, and describe why the changes are needed. Note that _new_ public APIs are not considered breaking. --> ## How was this change tested? Existing
## What changes are proposed in this pull request? The current struct expression patching API has several warts including: * No easy way to append a column at the end of the schema (must find/know last column and insert-after it) * Weird semantics of field replace and drop operations that change depending on invocation order * Awkward FFI support Rework the API to be easier to use and less error-prone and introduce a builder that can validate the changes before emitting a much simpler patch format. The builder exposes the following operations: * prepend (insert new field before-first) * append (insert new field after-last) * insert-after (some named predecessor) * replace (some named expression) * drop (error if target field is missing) * optional drop (ignore if target field is missing) * xxx_at (all of the above, but acting on some nested field) The builder's `build` method detects and rejects invalid combinations of operations such as: * double-drop, double-replace, or drop-and-replace * insert-after an optional drop (where to insert if target is missing?) * drop or replace a field with nested changes * no-op patch on a nested field The output of `build` is a simple patch format: * at struct level * prepended fields * appended fields * altered fields * at field level * keep_input - if true, propagate the named input to the output * insertions - a list of new fields to insert after the named input * optional - identifies an optional drop The FFI visitors were then rewritten to take advantage of this new simpler form (builders don't cross the FFI boundary). ### This PR affects the following public APIs Reworked the whole expression struct patch API. ## How was this change tested? Existing code updated to use the new builder, plus new tests.
## What changes are proposed in this pull request? Move declarative metadata plan construction onto `Scan`, so checkpoint, commit, normalization, and output-projection helpers access shared scan state through `self`. Split stats and partition-value normalization into focused helpers. This is a behavior-preserving refactor stacked on delta-io#3015. ## How was this change tested? - `cargo +nightly fmt --check` - workspace clippy with all features - workspace Arrow-only clippy without default features - `cargo doc --workspace --all-features --no-deps` - `cargo nextest run --workspace --all-features`
## What changes are proposed in this pull request? block dataChange removeFile for appendOnly tables <!-- **Uncomment** this section if there are any changes affecting public APIs. Else, **delete** this section. ### This PR affects the following public APIs If there are breaking changes, please ensure the `breaking-changes` label gets added by CI, and describe why the changes are needed. Note that _new_ public APIs are not considered breaking. --> ## How was this change tested? unit tests + integration tests over different batch index, selection vectors
## What changes are proposed in this pull request? The FFI schema visitor flattened all field metadata to strings via `StructField::metadata_with_string_values()`, so engines had to recover each value's real type from a hand-maintained allow-list of key names. Any metadata key whose value should be typed (e.g. a `Long`) silently came back as a string unless it was added to that allow-list. This threads the kernel's own type information across the boundary instead. A new `CMetadataMap` carries `MetadataValue`s (not strings), and the schema visitor callbacks receive `&CMetadataMap` in place of `&CStringMap`. Consumers read each value's type from a `CMetadataValueKind` tag rather than inferring it from the key. The value itself still crosses as a string (via `Display`) -- `MetadataValue::Other` is arbitrary JSON with no fixed C representation, so the string is the only universal form -- and the tag says how to interpret it. Two access patterns mirror `CStringMap`: `visit_metadata_map` (iterate all entries) and `get_from_metadata_map` (probe one key, with the kind returned via an out-parameter). Partition values keep `CStringMap`, since those are protocol-serialized strings by definition. The now-unused `StructField::metadata_with_string_values()` is removed. The in-tree `read-table` C example is updated to the new types. The C/JNR bridge and Java consumer that drop their `LONG_PROMOTED_KEYS` allow-list are a follow-up in the runtime repo. ### This PR affects the following public APIs Breaking FFI change: `EngineSchemaVisitor`'s `visit_*` callbacks now take `&CMetadataMap` instead of `&CStringMap`. Adds `CMetadataMap`, `CMetadataValueKind`, `visit_metadata_map`, and `get_from_metadata_map`. Removes `StructField::metadata_with_string_values()`. ## How was this change tested? Unit tests for `visit_metadata_map` (each `MetadataValue` variant round-trips to the expected kind tag and rendered string, including the empty-map case) and a discriminant-stability test pinning the `CMetadataValueKind` ABI values. The `read-table` C example compiles clean under the existing `-Werror` build against the regenerated header.
## What changes are proposed in this pull request? The sync engine was very hastily thrown together and agg evaluation in particular was not implemented well. Redo agg evaluation to handle any number and mix of either grouped or ungrouped aggs, and move it to its own module. Also clean up several other bits of warty code. ### This PR affects the following public APIs Agg enum changes from struct to tuple variants. ## How was this change tested? Missing unit tests added
…elta-io#3084) ## What changes are proposed in this pull request? Comment-only change. Expands the two-line comment at the `else` arm in `list_with_checkpoint_hint` to explain why the kernel returns `InvalidCheckpoint` rather than retrying the listing from version 0 when the `_last_checkpoint` hint points at a checkpoint that no longer exists. Listing only checks that the commits it finds are contiguous, not that they start at version 0. So two kinds of out-of-band log damage would otherwise replay into a plausible but wrong snapshot instead of an error: - a deleted log prefix leaving a contiguous suffix with no checkpoint, and - a drop-and-recreate at the same path mixing two tables' files. No behavior change: the `Err(Error::invalid_checkpoint(...))` is byte-identical to before. ## How was this change tested? No functional change, so no new tests. Existing coverage still passes: `cargo +nightly fmt --check`, `cargo clippy --all-features -- -D warnings`, and the full `cargo nextest run --workspace --all-features` suite (8450 passed) via the pre-commit hook.
…ta-io#3071) ## What changes are proposed in this pull request? The `column_expr!` and `col!` macros both accept the same kinds of args and produce column name expressions, but with subtly different semantics for some invocation patterns. Given `const FOO: &str = "foo"` and `const BAR_BAZ: &str = "bar.baz"`: |arg(s)|col! result|column_expr! result|comment |-|-|-|-| |`"a.b"`|`["a", "b"]`|`["a", "b"]`|Equivalent |`"a.b", "c"` | `["a.b", "c"]` | `["a", "b", "c"]` | `col!` only splits string literals on `.` for 1-arg invocation |`FOO`|`["foo"]`|`["foo"]`|`column_expr!` accepts `FOO` because it is contains simple path segment (no `.` or special chars) |`BAR_BAZ`|`["bar.baz"]`|compilation error|`column_expr!` rejects `BAR` because it contains `.` |`foo`|`[foo]`|compilation failure|`col!` performs runtime value interpolation with no visual syntax to request it Note: `column_expr!` originally supported only a single string literal arg. That limitation was especially painful for constant args and at least partly motivated the introduction of the `col!` macro. However, `column_expr!` now handles multiple arguments and also handles both string literals and constants, which brings it much closer in capabilities to `col!`. An audit of all `col!` and `column_expr!` call sites shows: |arg pattern|# col!|# col_expr!|comment |-|-|-|-| |single-arg simple string literal, e.g. `"x"`|526|567| |single-arg dot split string literal, e.g. `"a.b"`|8|32| |single-arg with constant arg, e.g. `FOO`|10|0| |single-arg with runtime arg, e.g. `name`|4|-|`column_expr!` forbids this case |multi-arg with all constant args, e.g. `FOO, BAR`|16|0| |multi-arg mixing constants and literals, e.g. `FOO, "a"`|1|1|The `col!` case is a doc test showing that `"a.b"` is not split |multi-arg involving runtime values, e.g. `"a", name`|0|-|`column_expr!` forbids this case This is good news, because the most ambiguous case (`col!("a.b", "c")` not splitting on dots) does not actually arise outside the doc test that demonstrates the corner case. Because the behavior of `col!` is generally more surprising (and less verifiable) than `column_expr!`, this PR changes the former into an alias of the latter. That allows everyone to use the shorter `col!` name but with clear semantics: * 1+ arguments supported * String literals containing dots are always split * Constant values are allowed, but only if they do not contain dots or other special characters (enforced at compilation time) * Runtime values are not allowed, because they cannot be validated. A selection of `column_expr!` sites are also changed to `col!`, to demonstrate the advantages of the shorter name. To avoid excessive churn in a behavior change PR, we leave a full sweep of the 600+ remaining `column_expr!` sites to future work. Future work could add proper runtime interpolation support to the macros, so that e.g. `col!({name})` and `col!({"a.b"})` would clearly indicate that the caller requests runtime interpolation for a single path segment, producing `Expression::column([name])` and `Expression::column(["a.b"])` respectively. This would allow to "reclaim" the four `col!` sites this PR had to rewrite as `Expression::column`. It would also allow to retire the `joined_column_*` family of macros in favor of e.g. `col!("a", {name}, FOO)` (which produces `Expression::column(["a", name, FOO])`. Future work could also consider removing the literal string-split capability altogether, since `col!("a.b.c")` can already be expressed by `col!("a", "b", "c")`. That said, the former easier to read and _much_ easier to type (`", "` requires pressing and releasing the shift key twice in three keystrokes). ### This PR affects the following public APIs `col!` macro changes behavior to match `column_expr!`, and runtime values are no longer accepted. Use `Expression::column` instead for those. The `column_expr!` macro remains usable, but is now `doc(hidden)` to steer new users toward the shorter name. ## How was this change tested? Compilation and lots of unit tests.
## What changes are proposed in this pull request? The log replay scan code exposes a weakness in `MaxNotNullBy` -- we really want a different column other than the returned value to control a row's eligibility, because the `add` returned by a scan may be NULL if a `remove` clobbered it. Today, the code has an ugly wrap/unwrap hack as a workaround. Fix the problem by expanding Max/MinNotNullBy aggs to take three args: a key to compare, a sentinel whose NULL values disqualify rows, and a (possibly NULL) value to return for the winning row. The log replay scan code can replace the hack with `MaxNotNullBy(add, file_key, version)`. ### This PR affects the following public APIs The Min/MaxNotNullBy aggs changed signature and semantics. ## How was this change tested? New and updated unit tests.
## What changes are proposed in this pull request? Follow-up to delta-io#3071, sweep the code base converting all `column_expr!` uses to `col!`. Also fix a few silly sites that were using `column_name!` in expression context or manually creating column names from literal strings, e.g. `Expression::column(["add"])`. ## How was this change tested? Compilation suffices, but unit tests still run.
## What changes are proposed in this pull request? Now that we have a generalized agg evaluator in the sync engine, define and implement SUM, COUNT(x) and COUNT(*) aggs. We'll need them eventually for things like rowid base calculations and CRC computations. ### This PR affects the following public APIs New agg types added. ## How was this change tested? New unit tests.
## What changes are proposed in this pull request? Removes version in kernel from datafusion executor to avoid having to bump version every new kernel vendor. ## How was this change tested? CI passes
## What changes are proposed in this pull request? This change allows for a `Kernel::Predicate` to be converted to a `DataFusion::Expr`. The reason behind this is that `DataFusion::Expr` includes predicate expressions, so any Kernel's predicate expression can be represented as a `DataFusion::Expr` as well. There was a divergence with current kernel implementation, so I made the change to the kernel here: delta-io#3085. ## How was this change tested? Tests added in new `predicate.rs`.
…a-io#3085) ## What changes are proposed in this pull request? ### What was the problem? Previously, `IN` membership against a literal array used physical equality. Physical equality treats two NULL scalar values as identical, so kernel returned `true` for `NULL IN [NULL, 2]`. That does not match logical SQL equality, where NULL is incomparable to every value, including itself. SQL represents the result as NULL (unknown). Kernel's `IN` predicate instead follows the convention of always returning a boolean, with an incomparable NULL producing `false`. The physical comparison appears to have been introduced in delta-io#1554. ### What is the solution? Use logical equality for membership against literal arrays. This prevents a NULL needle from matching a NULL element and makes literal-array behavior consistent with kernel's boolean convention: | Expression | SQL result | Kernel before | Kernel after | |---|---:|---:|---:| | `NULL IN [NULL, 2]` | NULL | `true` | `false` | | `NULL IN [2, 3]` | NULL | `false` | `false` | Logical comparison is not defined for struct, array, or map scalars. Those nested values therefore do not match, even when they are structurally identical. Removing the `IN` operator entirely can be considered separately. ## How was this change tested? Added unit coverage for literal-array and list-column membership, including present and absent values, NULL needles and elements, `NOT IN`, unsupported operand shapes, and nested scalar values.
…#3097) ## What changes are proposed in this pull request? A table upgraded to reader version 3 / writer version 7 can end up with `columnMapping` listed in `writerFeatures` but absent from `readerFeatures` (often an empty reader list). `Protocol::try_new` rejected this shape, so the kernel refused to load a table that delta-spark reads without complaint. Column-mapping mode is derived from `writerFeatures` (via `has_table_feature`), not from the reader list, so the table is genuinely readable: the empty reader list only tripped the up-front feature-consistency validation. This PR: - Exempts legacy ReaderWriter features from the "writer ReaderWriter feature must also be in readerFeatures" check in `Protocol::try_new`, and warns instead of failing. `ColumnMapping` is the only feature that qualifies (it is the sole entry in `LEGACY_READER_FEATURES`). - Teaches `TableConfiguration::is_feature_supported` to treat a legacy ReaderWriter feature as reader-supported when the protocol's reader version meets the feature's minimum, so support reporting agrees with the read path rather than splitting from it. This is a read-compatibility loosening: `Protocol::try_new` now accepts a protocol shape it previously rejected. ### Why lenient is also correct Reading the shape strictly (per PROTOCOL.md, "Reader Version 3 with the `columnMapping` table feature listed as supported") would treat column mapping as inactive, then resolve logical names against physical-named parquet and return nulls, i.e. silent data corruption. The literal spec wording is a `should`, not a `must`; the opposite shape (a feature in `readerFeatures` only) remains a hard error, as the spec requires. Deriving support from `writerFeatures` matches how delta-spark treats the shape. ### Invariants preserved - A feature present in neither list stays unsupported and inactive. - A non-legacy ReaderWriter feature (e.g. `deletionVectors`) missing from `readerFeatures` is still rejected. - A feature listed only in `readerFeatures` is still rejected. - Reader version 3 with `readerFeatures` absent entirely is still rejected (the field is required at v3). ## How was this change tested? - New read-path tests in `kernel/tests/integration/features/column_mapping_reader.rs`: a parametrized feature-support matrix (both-lists, orphaned, absent-from-both), and a parametrized end-to-end read for `name` and `id` mode that confirms values come back under the logical names, proving column mapping stays active rather than the load merely not erroring. - Unit tests in `table_configuration.rs` covering `is_feature_supported` for the orphaned shape, the conformant shape, and the absent-from-both case. - Negative tests for each rejected shape above. - `cargo test`, `cargo clippy --all-features -- -D warnings`, and `cargo fmt` all clean. --------- Co-authored-by: Sanuj Basu <sanujbasu@users.noreply.github.com>
## What changes are proposed in this pull request? Thread an optional output_type through expression conversion so the struct-shaped arms can lower against a target schema. Implements `Struct` and `StructPatch` expressions. - Optional `output_type` is only forwarded for `Coalesce` and `Array` cases. ## How was this change tested? Tests within PR
## What changes are proposed in this pull request? - Consolidate overlapping `scan_metadata` structured-stats tests into a named five-case `rstest` matrix. - Cover one and multiple requested stat columns, predicates on requested and unrequested columns, no predicate, and predicates selecting no files. - Assert exact selected min/max ranges, `numRecords`, requested min/max/null-count fields, and exact projection when no predicate requires internal stats. - Update the `parsed-stats` fixture catalog. ## How was this change tested? Existing tests
## What changes are proposed in this pull request? Convert `MapToStruct` expressions to DataFusion expressions. Arrow casting has two known divergences from kernel casting: - Booleans accept additional values such as `"yes"`. - Decimals are implicitly rescaled, while kernel returns an error for a scale mismatch. ## How was this change tested? Added focused expression-lowering tests and ran the full root and `datafusion-executor` test suites.
## What changes are proposed in this pull request? This adds `ParseJson` as a UDF to Kernel. The changes were made using a custom DataFusion UDF and existing Kernel parse_json. This was to ensure compatibility with Kernel parse_json. There was consideration of using the third party [datafusion-functions-json](https://github.com/datafusion-contrib/datafusion-functions-json), but I believe Kernel has better compatibility now. If there are any performance / optimization issues with using this custom UDF, a change could be made in the future. For comparison with datafusion-functions-json: 1. Decimal in Kernel is implemented differently, so it would have to be a `json_get_str` and `try_cast` 2. Date/Time not implemented at all, so similarly `json_get_str` and `try_cast` 3. Need to reconstruct the Struct each field at a time, no generic json -> struct. Each field will need separate `json_get` calls that walks through the same json bytes again (O(N^2)). Not sure about performance here. For reference, Kernel reads once and then casts each field separately. ## How was this change tested? This change was tested by directly evaluating some json formats.
## 🥞 Stacked PR Use this [link](https://github.com/delta-io/delta-kernel-rs/pull/3026/files) to review incremental changes. - [stack/validate-require-field](delta-io#2856) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2856/files)] [MERGED] - [stack/addFile-part-val](delta-io#2939) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2939/files)] [MERGED] - [**stack/dv-val**](delta-io#3026) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/3026/files)] ← _this PR_ - [stack/validate-required-field-removeFile](delta-io#2974) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2974/files)] --------- ## What changes are proposed in this pull request? Currently kernel already has some validations for staged addFile but not staged dv. This PR is to apply same level of validations on staged DV. Including - Required fields existence - Partititon keys correctness ## How was this change tested? tests for all negative cases
…elta-io#3109) ## What changes are proposed in this pull request? <!-- **Uncomment** this section if there are any changes affecting public APIs. Else, **delete** this section. ### This PR affects the following public APIs If there are breaking changes, please ensure the `breaking-changes` label gets added by CI, and describe why the changes are needed. Note that _new_ public APIs are not considered breaking. --> Super quick one. Seems like there are spurious network issues causing `PeerDisconnected` failures on certain CI runs. This seems like Github Runner instability, so applied some simple retries to both fetches. ureq doesn't expose any native primitives to perform automated retries. Example: https://github.com/delta-io/delta-kernel-rs/actions/runs/31638152264/job/94253598747?pr=3105 Build failures (superset of them): https://github.com/delta-io/delta-kernel-rs/actions?query=is%3Afailure+workflow%3Abuild ## How was this change tested? Cycled CI 6 times and observed no failures. I was previously observing failures in ~1/3 of the CI runs. Co-authored-by: jtuglu1 <jtuglu1@users.noreply.github.com>
…ta-io#3095) ## What changes are proposed in this pull request? We could already derive a kernel schema from a POD struct. Now we can use the same proc macro infra to derive `impl From<MyStruct> for Scalar`. Make array and map extraction infallible while we're at it. The same derivation allows a new idiomatic `PlanBuilder::from_values([a, b, c])`, which creates a `Values` node from the fields of input values that are convertible to `StructData`, with schema provided by `ToSchema`. ### This PR affects the following public APIs Converting `Vec` and `HashMap` to `Scalar::Array` and `Scalar::Map` is no longer fallible. ## How was this change tested? New and existing unit tests.
Release v0.27.0 -- version bump (0.26.0 -> 0.27.0) and changelog for `delta_kernel`, `delta_kernel_derive`, and `delta_kernel_default_engine`. See the CHANGELOG.md diff for the full list of changes since v0.26.0, including the Breaking changes section.
…io#3101) ## What changes are proposed in this pull request? Building on delta-io#3095, add derive-macro support for `TryFrom<Scalar> for T`, where `T` is some rust struct. This allows a full round trip from `Vec<Foo>` to `Operation::Values` and back. ### This PR affects the following public APIs New `TryFrom<Scalar>` capability, plus supporting derive macro. `StructData::into_values` replaced with (safer) `StructData::into_parts` that returns field info as well. ## How was this change tested? New unit tests.
…io#3121) ## What changes are proposed in this pull request? Sweep the code base for opportunities to simplify column name manipulation. Some of this is tech debt created when the `column_name!` family of macros recently became more capable, but a lot of it was just plain missed opportunity. Find and fix them all. ## How was this change tested? Mechanical change, covered by compilation and unit tests
delta-io#3123) `schema/mod.rs` imported `CollectInto` from the crate root, but that re-export is gated behind the `internal-api` feature. `cargo publish` verifies the packaged crate with default features (`delta_kernel`'s `default = []`), so the crate failed to compile without `internal-api` (E0432 unresolved import) -- which blocked the v0.27.0 publish. The `--all-features` CI build enabled `internal-api`, so it never exercised this path. Fix: import `CollectInto` from `crate::utils` (where it's defined), which resolves regardless of feature flags. Verified with `cargo check -p delta_kernel --no-default-features`: compiles with 0 errors (previously E0432).
Release v0.27.1 -- version bump (0.27.0 -> 0.27.1) and changelog for `delta_kernel`, `delta_kernel_derive`, and `delta_kernel_default_engine`. v0.27.0 was tagged but never published to crates.io (its publish verify failed on a default-feature build). This release supersedes it: the changelog folds everything since v0.26.0 into a single v0.27.1 section (including the commits that landed after v0.27.0 was cut). Pairs with the compile fix in delta-io#3123, which must merge before v0.27.1 is tagged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
How was this change tested?