Store package names in arrow metadata #122
Conversation
ericphanson
requested review from
jrevels,
omus and
kleinschmidt
as code owners
|
good call @kleinschmidt, I've switched this to use the schema-version instead of the schema name (defined in the |
|
@ericphanson you probably also want to include the version of the package as well since older or newer versions may be incompatible. |
| # Let's test some more error printing while we're here; if we did not have the VersionNumber | ||
| # (e.g. since the table was generated on Julia pre-1.9), we should still print a reasonable message: | ||
| err = Legolas.UnknownSchemaVersionError(Legolas.SchemaVersion("test-provider-pkg.foo", 1), :TestProviderPkg, missing) | ||
| @test contains(sprint(Base.showerror, err), "TestProviderPkg") |
There was a problem hiding this comment.
We should use Arrow.Table to load the table without the package to ensure that the advice we give is accurate
Co-authored-by: Curtis Vogt <[email protected]>
| v = Legolas.extract_metadata(table, Legolas.LEGOLAS_SCHEMA_PROVIDER_NAME_METADATA_KEY) | ||
| @test v == "TestProviderPkg" |
| # Check if this module was defined in a package. | ||
| # If not, return `nothing` | ||
| path = pathof(rootmodule) | ||
| path === nothing && return (; name=nothing, version=nothing) |
There was a problem hiding this comment.
Comment above makes it sound like this should be:
| path === nothing && return (; name=nothing, version=nothing) | |
| path === nothing && return nothing |
There was a problem hiding this comment.
needs to be consistent with the return value described in the docstring of schema_provider though, which is as it is here...
| schema_metadata = LEGOLAS_SCHEMA_QUALIFIED_METADATA_KEY => identifier(sv) | ||
| provider_name, provider_version = schema_provider(sv) | ||
| provider_name_metadata = LEGOLAS_SCHEMA_PROVIDER_NAME_METADATA_KEY => string(provider_name) | ||
| provider_version_metadata = LEGOLAS_SCHEMA_PROVIDER_VERSION_METADATA_KEY => string(provider_version) | ||
| if isnothing(metadata) | ||
| metadata = (schema_metadata,) | ||
| if !isnothing(provider_name) | ||
| metadata = (metadata..., provider_name_metadata) | ||
| if !isnothing(provider_version) | ||
| metadata = (metadata..., provider_version_metadata) | ||
| end | ||
| end | ||
| else | ||
| metadata = Set(metadata) | ||
| push!(metadata, schema_metadata) | ||
| if !isnothing(provider_name) | ||
| push!(metadata, provider_name_metadata) | ||
| if !isnothing(provider_version) | ||
| push!(metadata, provider_version_metadata) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
A cleaner implementation. Assumes that schema_provider returns nothing when there is no provider. Additionally, note that Arrow.getmetadata returns an ImmutableDict{String,String}:
| schema_metadata = LEGOLAS_SCHEMA_QUALIFIED_METADATA_KEY => identifier(sv) | |
| provider_name, provider_version = schema_provider(sv) | |
| provider_name_metadata = LEGOLAS_SCHEMA_PROVIDER_NAME_METADATA_KEY => string(provider_name) | |
| provider_version_metadata = LEGOLAS_SCHEMA_PROVIDER_VERSION_METADATA_KEY => string(provider_version) | |
| if isnothing(metadata) | |
| metadata = (schema_metadata,) | |
| if !isnothing(provider_name) | |
| metadata = (metadata..., provider_name_metadata) | |
| if !isnothing(provider_version) | |
| metadata = (metadata..., provider_version_metadata) | |
| end | |
| end | |
| else | |
| metadata = Set(metadata) | |
| push!(metadata, schema_metadata) | |
| if !isnothing(provider_name) | |
| push!(metadata, provider_name_metadata) | |
| if !isnothing(provider_version) | |
| push!(metadata, provider_version_metadata) | |
| end | |
| end | |
| end | |
| metadata = Set{String,String}(isnothing(metadata) ? [] : metadata) | |
| push!(metadata, LEGOLAS_SCHEMA_QUALIFIED_METADATA_KEY => identifier(sv)) | |
| provider = schema_provider(sv) | |
| if !isnothing(provider) | |
| push!(metadata, LEGOLAS_SCHEMA_PROVIDER_NAME_METADATA_KEY => string(provider_name)) | |
| push!(metadata, LEGOLAS_SCHEMA_PROVIDER_VERSION_METADATA_KEY => string(provider_version)) | |
| end |
There was a problem hiding this comment.
I think I agree with this, and it's easier for me to follow at least
There was a problem hiding this comment.
mostly agree with @omus review and have left comments where I don't. needs a version bump.
| v = Legolas.extract_metadata(table, Legolas.LEGOLAS_SCHEMA_PROVIDER_NAME_METADATA_KEY) | ||
| @test v == "TestProviderPkg" |
| # Check if this module was defined in a package. | ||
| # If not, return `nothing` | ||
| path = pathof(rootmodule) | ||
| path === nothing && return (; name=nothing, version=nothing) |
There was a problem hiding this comment.
needs to be consistent with the return value described in the docstring of schema_provider though, which is as it is here...
| schema_metadata = LEGOLAS_SCHEMA_QUALIFIED_METADATA_KEY => identifier(sv) | ||
| provider_name, provider_version = schema_provider(sv) | ||
| provider_name_metadata = LEGOLAS_SCHEMA_PROVIDER_NAME_METADATA_KEY => string(provider_name) | ||
| provider_version_metadata = LEGOLAS_SCHEMA_PROVIDER_VERSION_METADATA_KEY => string(provider_version) | ||
| if isnothing(metadata) | ||
| metadata = (schema_metadata,) | ||
| if !isnothing(provider_name) | ||
| metadata = (metadata..., provider_name_metadata) | ||
| if !isnothing(provider_version) | ||
| metadata = (metadata..., provider_version_metadata) | ||
| end | ||
| end | ||
| else | ||
| metadata = Set(metadata) | ||
| push!(metadata, schema_metadata) | ||
| if !isnothing(provider_name) | ||
| push!(metadata, provider_name_metadata) | ||
| if !isnothing(provider_version) | ||
| push!(metadata, provider_version_metadata) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
I think I agree with this, and it's easier for me to follow at least
for more informative unknown schema errors
closes #46
Here I chose to only store this information when the schema was defined in a package (rather than in Main or in a script or something), since it's only in that case we can reliably make a suggestion to load that package.