Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move node dataset resolver subquery ids into a separate namespace #1280

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

tlento
Copy link
Contributor

@tlento tlento commented Jun 14, 2024

Move node dataset resolver subquery ids into a separate namespace

The DataflowPlanNodeOutputDataSetResolver class extends the
DataflowToSqlQueryPlanConverter class. As a result, both classes
effectively do a full conversion from a DataflowPlan to a SqlQueryPlan,
complete with SqlQueryPlan node_id generation. These shared the
same ID prefix, which meant any call that might trigger an additional
invocation of the node dataset resolver subclass could cause shifts
in the subquery IDs in our SqlQueryPlan outputs even if the invocation
had no material impact on the query in question.

In order to reduce snapshot thrash and make our subquery IDs easier to
reason about on read-through of the generated SQL, this change splits
the node dataset resolver ID prefix into a dedicated value.

Update engine snapshots

Copy link
Contributor Author

tlento commented Jun 14, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlento and the rest of your teammates on Graphite Graphite

Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@tlento tlento added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 14, 2024
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:21 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:21 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:21 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:21 — with GitHub Actions Inactive
@tlento tlento marked this pull request as ready for review June 14, 2024 00:22
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 14, 2024
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:26 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:26 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:26 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 14, 2024 00:26 — with GitHub Actions Inactive
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helpful thank you!

Copy link
Contributor Author

tlento commented Jun 25, 2024

Merge activity

  • Jun 24, 8:29 PM PDT: @tlento started a stack merge that includes this pull request via Graphite.
  • Jun 24, 9:02 PM PDT: Graphite rebased this pull request as part of a merge.
  • Jun 24, 9:06 PM PDT: @tlento merged this pull request with Graphite.

@tlento tlento force-pushed the evaluate-columns-for-pushdown branch from cd70c5d to 6b94906 Compare June 25, 2024 03:57
Base automatically changed from evaluate-columns-for-pushdown to main June 25, 2024 04:01
The DataflowPlanNodeOutputDataSetResolver class extends the
DataflowToSqlQueryPlanConverter class. As a result, both classes
effectively do a full conversion from a DataflowPlan to a SqlQueryPlan,
complete with SqlQueryPlan node_id generation. These shared the
same ID prefix, which meant any call that might trigger an additional
invocation of the node dataset resolver subclass could cause shifts
in the subquery IDs in our SqlQueryPlan outputs even if the invocation
had no material impact on the query in question.

In order to reduce snapshot thrash and make our subquery IDs easier to
reason about on read-through of the generated SQL, this change splits
the node dataset resolver ID prefix into a dedicated value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants