refactor(catalog): unify name resolution with SchemaResolver trait #1492
Conversation
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
Consolidate four nearly-identical table/function resolution implementations into a single `resolve_logical_catalog` function with a `SchemaResolver` trait that abstracts over different resolution strategies: - `DynamicResolver`: For user queries (resolves via store) - `PreResolvedResolver`: For derived datasets (looks up in dependencies map) Key changes: - Add `SelfReferences` type for self.function() resolution in derived datasets - Nest `ResolveError` in `Resolution` variant to preserve error types for correct HTTP status code mapping (400 for alias not found, 404 for function/table not found) - Use `error_with_causes` in admin-api for full error chain in responses - Add `schema` field to `FunctionNotFound` for better error messages (shows 'eth.func' instead of just 'func') - Fix unqualified table detection when flattening references This eliminates ~200 lines of duplicated resolution logic while maintaining proper error semantics.
LNSD
reviewed
Dec 19, 2025
LNSD
reviewed
Dec 19, 2025
LNSD
reviewed
Dec 19, 2025
LNSD
reviewed
Dec 19, 2025
| /// This trait provides the minimal interface required for SQL catalog building, | ||
| /// abstracting over the dataset store implementation. | ||
| pub trait DatasetAccess { | ||
| pub trait DatasetAccess: Send + Sync + 'static { |
Contributor
There was a problem hiding this comment.
Are these necessary? Can't we just require them in the where clause, or let the compiler infer them?
Collaborator
Author
There was a problem hiding this comment.
We can put them in the where clause, but I have the habit of doing this for syntactical convenience. It's an internal trait so we can change whenever.
More descriptive name that clarifies this resolver is used for registry-based schema resolution (user queries via the dataset store).
Reorganize the file to put the main public function near the top, after its type dependencies. The implementation-specific RegistrySchemaResolver is moved to the bottom.
When an error variant has #[source], including {0} in the message
duplicates the source error. The source is automatically included
in the error chain via error_with_causes.
Collaborator
Author
|
@LNSD all comments addressed afaict |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Consolidate four nearly-identical table/function resolution implementations into a single
resolve_logical_catalogfunction with aSchemaResolvertrait that abstracts over different resolution strategies:DynamicResolver: For user queries (resolves via store)PreResolvedResolver: For derived datasets (looks up in dependencies map)Key changes:
SelfReferencestype for self.function() resolution in derived datasetserror_with_causesin admin-api for full error chain in responsesThis eliminates ~200 lines of duplicated resolution logic while maintaining proper error semantics.