Repository navigation
Align table handles and error codes with Trino conventions - #358
Merged
Merged
Conversation
Without equals and hashCode, two handles describing the same table, filters, and limit were only equal if they were the same instance, so handles deserialized on another node, or created again by the engine, were never equal to the original. Column handles and handles of upstream connectors compare every field, and table handles should follow the same contract.
The engine uses the retained size of handles to account for the memory of queued splits, but the size of a pushed down sort item only included its column name. The test checks that every field adds to the size, so fields added later are not left out.
Every split looked up each projected column by comparing its name with the names of all table columns, ignoring case, and built the table metadata only to read those names. Rows have values of all table columns in the order the spec defines them, and projected columns are handles of those same columns, so a map from handles to positions finds them without scanning, and without depending on how names are cased.
Problems found while mapping the parsed spec to tables, and while resolving its servers, were thrown as IllegalArgumentException, which Trino reports as GENERIC_INTERNAL_ERROR, as if the connector had a bug. Other connectors report invalid external metadata with their own error code, so problems in the spec use OPENAPI_INVALID_SPEC, like the parser errors already do. Server problems that are fixed by setting base-uri or authentication.allow-insecure-http are catalog configuration problems, so they use INVALID_CATALOG_PROPERTY, like the authentication problems. Pagination, sorting, and range filter options still validate their own arguments, and the spec wraps their errors, because they are only created from the spec.
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
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.
These are differences from Trino conventions that reviewers would ask about when the connector is contributed upstream (#205).
ExampleTableHandleandPrometheusTableHandle, compare by value, so table handles now do too.sortItem,rowFilter, andspecHashwere already counted in the retained size, but theSortItemobject itself wasn't. The engine uses this size to account for the memory of queued splits. The new test checks that every field adds to the size, so fields added later aren't left out.IllegalArgumentException. Trino reports that asGENERIC_INTERNAL_ERROR, as if the connector had a bug. Spec problems now useOPENAPI_INVALID_SPEC(Load the spec on every node reliably and detect mismatches #342). Server problems that are fixed by settingbase-uriorauthentication.allow-insecure-httpuseINVALID_CATALOG_PROPERTY, like the authentication problems already do. The error code offset0x0700_0000doesn't change here. An upstream contribution would get a new offset from the range of upstream connectors.Left for later:
DecompressingHttpClient, the disk HTTP cache,RetryPolicy).io.trino.plugin.openapipackage, thetrino-rootparent, and removing the release configuration. This only makes sense in the upstream PR.Part of #300