Repository navigation
feat: add IcebergCatalogTableProvider::new_with_schema - #48
NoahKusaba wants to merge 1 commit into
Conversation
| /// assert_eq!(rebuilt.schema(), original.schema()); | ||
| /// # Ok::<(), Box<dyn std::error::Error>>(()) | ||
| /// # })?; | ||
| /// # Ok::<(), Box<dyn std::error::Error>>(()) |
There was a problem hiding this comment.
This example here seems unreasonably long and verbose for a method that is just:
pub fn new_with_schema(
catalog: Arc<dyn Catalog>,
table_ident: TableIdent,
schema: ArrowSchemaRef,
) -> Self {
IcebergTableProvider {
catalog,
table_ident,
schema,
})
}TBH, for a public method like this, I don't think it needs more than a couple of lines of docs. An explanation of how IcebergTableProvider works is better given as a doc comment of the IcebergTableProvider struct itself, rather than a constructor method.
| let expected = "+----+------+\n\ | ||
| | id | name |\n\ | ||
| +----+------+\n\ | ||
| | 1 | a |\n\ | ||
| +----+------+"; | ||
| assert_eq!(query(&rebuilt_ctx, "SELECT * FROM t").await, expected); | ||
| assert_eq!(query(&ctx, "SELECT * FROM t").await, expected); |
There was a problem hiding this comment.
We should not do these kind of expectations here, prefer using insta so that expectations over ascii graphics are automatically managed.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks @NoahKusaba. The constructor is the right shape for a codec. datafusion-proto's LogicalExtensionCodec::try_decode_table_provider is synchronous and is handed the serialized schema. The decoder resolves the scan's projection against that schema, then applies those indices to the rebuilt provider's schema(). So the rebuilt provider has to declare exactly the original's schema, and it can't load the table there without blocking. It would help to say that in the description, because it answers "why not call try_new on the scheduler?".
A few things before this can merge:
1. Rebase onto main. #49 renamed the provider to IcebergCatalogTableProvider and moved it to table_provider/catalog_table_provider.rs, so this PR now conflicts. The doctest's use datafusion_iceberg::IcebergTableProvider no longer resolves. The port is mechanical. I tried it locally, and fmt, clippy, rustdoc and the full test suite pass on top of main.
2. Docs. I agree with @gabotechs that the example can go. It's mostly hidden setup to show that a three-field constructor round-trips, and #50 already cut the similar example on new_with_predicate down to a few lines. Two things in the text are also off:
- "A name the table lacks fails the scan" should refer to the schema of the snapshot being scanned, not the table's current schema (#42). For example, a provider built with
try_newafter a column is added, but before any write, fails withColumn email not found in table, even though the table hasemail. - Nothing checks
schemaagainst the table, and a mismatch doesn't fail cleanly. Withid: Int64declared over anInt32column,SELECT id + 1panics in arrow (primitive array). The docs should say the schema isn't validated, rather than imply that it's safe "even after the table's schema has changed".
Something like:
/// Creates a provider for `table_ident` that declares `schema`, without loading
/// the table. For rebuilding a provider in another process from [`Self::catalog`],
/// [`Self::table_ident`] and [`TableProvider::schema`]. `schema` is not checked
/// against the table; scans read its columns by name from the snapshot they scan.
3. Tests. For the snapshot assertions: the workspace doesn't depend on insta, but expect-test is already a dev-dependency and the integration tests use expect![[...]], so that avoids adding a dependency. The integration tests also already cover most of this:
test_plan_nodes_are_inspectablerebuilds a provider from its accessors withtry_new. With this PR, a codec would make that call withnew_with_schemainstead.test_scan_after_schema_evolution_reads_provider_columnsbuilds a provider, adds a column, and scans.
Switching that rebuild to new_with_schema, and scanning through a rebuilt provider at the end of the evolution test, would cover this in a few lines. That would replace the new unit test, the query helper and the doctest.
4. Heads-up, not for this PR. A rebuilt provider inherits the cached-schema bugs filed as #56 and #57. After a column is added to the table, INSERTs through a provider built before the change fail. After a column is dropped and added back under the same name, IS NULL counts come out wrong, and queries panic if the type changed. Ballista runs DML through the rebuilt provider, so it will hit these as soon as another engine changes a table's schema.
Nit: the PR title needs the new type name after the rebase.
|
Thanks for the review @andygrove, I was going to wait for #18 to also merge (along with the other refactors), but to be fair it isn't a real blocker so I've addressed all of the comments in the new commit. Small note on the doc example I provided that is now removed, I was asked to provide one for public api's in previous PR's, but I agree it's unnecessary. I'll start working on the standalone table-scan PR, and will hopefully have the PR up on Ballista |
76d5c61 to
0eb5e78
Compare
NoahKusaba
changed the title
|
Just merged #18! about the snapshot testing, DataFusion uses For consistency with Apache DataFusion, I think we should move this project out of |
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
LGTM, thanks @NoahKusaba!
@gabotechs, I agree on moving to insta for consistency with DataFusion. It probably fits best in a follow-up that converts the existing expect! snapshots, rather than in this PR.
Also, the files that #18 added are missing the ASF license header, which the RAT check in #32 will flag. Could you add it to these?
crates/datafusion/src/test_utils/mod.rscrates/datafusion/src/test_utils/harness.rscrates/datafusion/tests/external_table.rscrates/datafusion/tests/filter_pushdown.rscrates/datafusion/tests/limit_pushdown.rscrates/datafusion/tests/projection_pushdown.rscrates/datafusion/testdata/iceberg/taxi/README.md
I also created a separate PR split out of #32 to add the RAT checks. #60 |
|
Can we merge this in? I'll create the follow up test fix in the next PR. |
Builds a catalog-backed provider from its catalog, table identifier and schema without loading the table, so a codec can rebuild a provider in another process that declares exactly the original's schema.
0eb5e78 to
c8c9171
Compare
|
cc @gabotechs |
gabotechs
left a comment
gabotechs
left a comment
There was a problem hiding this comment.
+1 for unblocking, but I have to say I don't love that we make inconsistent state representible in the public API
| pub fn new_with_schema( | ||
| catalog: Arc<dyn Catalog>, | ||
| table_ident: TableIdent, | ||
| schema: ArrowSchemaRef, | ||
| ) -> Self { | ||
| IcebergCatalogTableProvider { |
There was a problem hiding this comment.
I don't love the fact that the public API of this project makes incorrect state representable.
People here can choose to shoot themselves in the foot and pass a schema that has nothing to do with the actual table. This problem goes a bit beyond just this new constructor method, and it's rooted into the IcebergCatalogTableProvider struct fields themselves.
One thing I notice is that IcebergCatalogTableProvider only needs the Catalog itself because it's required for constructing an IcebergCommitExec. It's a bit unfortunate that Catalog is dangerously close to a god object in charge of everything, some interface segregation there could have prevented from needing to thread it around everywhere.
Given that Catalog contains so many methods and not necessarily related functionality, I wonder if it'd make sense to have it be part of the DataFusion's SessionConfig.extensions and thread it around as part of the context, rather than manually requiring it to be stored in every struct.
|
There both seems to be eagerness to merge this, and concern about public API.
Is it in scope for this PR, or do we need to file issues to follow up? I don't want this to get lost and am catching up on PRs on this repo. |
If nobody else gets to it, I'm happy to look into how we can create a more elegant interface, as I agree with Gabotechs assessment. I've been a bit tunnel-visioned on just opening publicized functions for Ballista to hook into, and it's better to get this right at the start than refactor after the fact. |
I'm okay merging this as-is as long as the discussion about the API is summarized in an issue that we can track and address before cutting the first release. |
|
I'm fine with merging this as-is now if that helps people get unblocked, as I unfortunately do not have a better suggestion of how to structure the API. Exposing a clean API will likely will likely have a domino effect in which we'll need to change the fields of several I think it's worth exploring that and getting to the right structure will likely require some hands-on experimentation, but again, don't want to block other efforts so I'm fine with merging this as-is (probably with some issue created for tracking this). |
|
Rethinking the design, I don't think this new_with_schema is needed |
Which issue does this PR close?
Rationale for this change
A distributed engine like Ballista serializes a catalog-backed
IcebergCatalogTableProviderin one process and rebuilds it in another, such as its scheduler. #20 exposes the provider's parts (catalog(),table_ident()andschema()). The only public constructor,try_new, reloads the table and takes the schema the table has now.A codec can't use
try_newfor the rebuild. datafusion-proto'sLogicalExtensionCodec::try_decode_table_provideris synchronous and is handed the serialized schema. The decoder resolves the scan's projection against that schema, then applies the resulting indices to the rebuilt provider'sschema(). So the rebuilt provider has to declare exactly the original's schema, and it can't load the table there without blocking.What changes are included in this PR?
IcebergCatalogTableProvider::new_with_schema(catalog, table_ident, schema). It builds a provider that declaresschema, without loading the table.schemais not checked against the table.try_newnow loads the schema and delegates tonew_with_schema, and its docs gain an# Errorssection.Are there any user-facing changes?
One new public constructor. Nothing breaks.
Are these changes tested?
Yes, by extending existing integration tests:
test_plan_nodes_are_inspectablenow rebuilds the provider from its accessors withnew_with_schema, the way a codec would.test_scan_after_schema_evolution_reads_provider_columnsalso scans through a provider rebuilt after the column is added. It returns the same columns as the original.