transform: add ColumnNames attribute
#21796
Merged
Conversation
This file contains 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
|
This PR has higher risk. In addition to having a knowledgeable reviewer, it may be useful to add observability and/or a feature flag. What's This?
|
aalexandrov
added
A-optimization
5 tasks
vmarcos
reviewed
Sep 18, 2023
There was a problem hiding this comment.
This looks good to me, with the caveat that I do not have expertise in the attribute derivation framework (and just studied the necessary parts for reviewing). Overall, moving away from the dependency on typemap_rev seemed to be a positive development regardless, though, and I think that the refactor is amply justified.
There were a few minor questions I had while reviewing, and we can discuss below if any further changes to address those make sense or not.
Change the `DefSource` directive syntax from ``` DefSource name=t0 keys=[[#0]] - bigint - bigint? ``` to ``` DefSource name=t0 keys=[[#0]] - c0: bigint - c1: bigint? ``` so the `Humanizer` implementation for the `TestCatalog` can properly answer `column_names_for_id` calls.
Use ``` explain with=(arity,types) ``` To output the explained plan with `arity` and `types` attribute annotations. This also adds separate tests for `arity` and `types` to validate that the new directive works as expected based on some existing attributes.
The `DerivedAttributes` and `RequiredAttributes` structs were hosting instances of the attribute derivation algorithms for the attributes to be derived in `TypeMap` instances. This is preventing us from seeding the algorithm (`Attribute`) instances with `&dyn ExprHumanizer` context because `TypeMap` keys need to be thread-safe (`Send + Sync`). In order to allow `Attribute` implementations to depend on context types such as `&dyn ExprHumanizer` or `&dyn StatisticsOracle` we replace the `TypeMap` fields with a custom struct called `AttributeStore`. The struct has an `Option<T>` field provisioned for each `A` that implements `Attribute`. Fields are listed in dependency order and methods that need to respect the dependency order access them in the same sequence. An implementation of `AttributeContainer<A>` for each field provides a unified interfacing for accessin the attribute of type `A` from client code. See the rustdoc comments on these items for more details. The change removes the following code from the `attribute` framework: - The `typemap_rev` crate dependency (which provided `TypeMap`). - The `AsKey` helper struct. - The `AttributeId` enum and the `TOTAL_ATTRIBUTES` const field.
Add a `ColumnNames` attribute and expose it in `EXPLAIN` outputs through the `column_names` option.
aalexandrov
force-pushed
the
column_names_attribute
branch
from
647740d
to
c0e08b4
Compare
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.
Add a
ColumnNamesattribute and expose it inEXPLAINoutputs through thecolumn_namesoption.Motivation
We don't have an issue for that, but multiple people have expressed interest in having an
EXPLAINplan that prints humanized column names. This PR is the first step towards that.Tips for reviewer
See commit messages for details (specifically the message of the third commit for motivation and background of the refactoring).
The first four commits do some refactoring to the testing and attribute derivation frameworks required that is needed in order to implement the
ColumnNamesinference logic. More specifically, we need a way to createAttributeimplementations with parameters (until now the structs implementingAttributewere all nullary). This is achieved by the third and fourth commits which:DerivedAttributescontainer.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.