Sync streams: Support aliases #377
Merged
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.
As reported on Discord, using sync streams with a subquery having a table alias results in now rows being synced.
Sync rules avoid this issue by making aliases in parameter queries illegal and checking for that. We could do the same for sync streams, but that feels like an unecessary restriction. The actual bug here is that it's not consistently clear whether a
tableName: string
refers to the name in the source schema or the aliased name it has when parsing expressions. Because sync streams reference the aliased name when checking whether a given row can be a parameter input, this caused input tables to not be recognized.This refactors
SqlTools
and related structures insync-rules
to always be aware of whether a name refers to an alias or the source table. When looking up values to create parameter lookups or bucket ids, we should always use the schema name. For output names, we use aliased names. By introducing theAvailableTable
class for tables that have been added to a result set viaFROM
, we're guaranteed to always have both names available.This could be used to lift the restriction of aliases for sync rules, but that feels unecessary given the eventual migration to sync streams.