Skip to content

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented Jun 23, 2021

Which issue does this PR close?

Closes #.

Rationale for this change

  1. alias map building can be reused
  2. resolve_positions_to_exprs should return Option not Result

What changes are included in this PR?

reuse alias map in aggregate logical planning and refactor position resolution

Are there any user-facing changes?

@jimexist jimexist changed the title reuse alias map in aggregate logical planning and refactor position r… reuse alias map in aggregate logical planning and refactor position resolution Jun 23, 2021
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @jimexist this looks like a nice cleanup -- can you confirm my understanding that this is a refactoring only (aka no behavior change)?

It looks like the PR stops (re)computing extract_aliases by doing so once and reusing the results

@jimexist
Copy link
Member Author

jimexist commented Jun 23, 2021

It looks like the PR stops (re)computing extract_aliases by doing so once and reusing the results

yes

Thank you @jimexist this looks like a nice cleanup -- can you confirm my understanding that this is a refactoring only (aka no behavior change)?

yes and i believe all untouched unit tests and integration tests shall vet for that

@alamb
Copy link
Contributor

alamb commented Jun 23, 2021

yes and i believe all untouched unit tests and integration tests shall vet for that

I agree

@jimexist jimexist force-pushed the aggregate-reuse-alias-map branch from ba6102f to a40f0b0 Compare June 23, 2021 15:34
@alamb alamb merged commit aead7f8 into apache:master Jun 24, 2021
@jimexist jimexist deleted the aggregate-reuse-alias-map branch June 24, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants