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.

Sign up for GitHub

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

Jump to bottom

Remove parentheses from create view logic #172

Merged
merged 4 commits into from
Jul 14, 2023
Merged

Remove parentheses from create view logic #172

merged 4 commits into from
Jul 14, 2023

Conversation

ravjotbrar
Copy link
Contributor

@ravjotbrar ravjotbrar commented Apr 4, 2023 •
edited
Loading

Summary

Remove parentheses from view materialization to optimize VDSs in Dremio.

Description

Before:

(
   select * from table
)

After:

select * from table

Test Results

All functional tests pass.

Changelog

  • Added a summary of what this PR accomplishes to CHANGELOG.md

Related Issue

#167

@ravjotbrar ravjotbrar requested review from ArgusLi and jlarue26 April 4, 2023 20:56
ArgusLi
ArgusLi reviewed Apr 4, 2023
View reviewed changes
Copy link
Contributor

@ArgusLi ArgusLi left a comment

Choose a reason for hiding this comment

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

Should the changelog be updated as well?

@ArgusLi
Copy link
Contributor

ArgusLi commented Apr 5, 2023

Sorry to nit, but should this be filed under Under the hood instead? I don't think it's actually fixing anything but just leading to a performance increase.

@ravjotbrar
Copy link
Contributor Author

Sorry to nit, but should this be filed under Under the hood instead? I don't think it's actually fixing anything but just leading to a performance increase.

Good call. Made the change :)

ArgusLi
ArgusLi approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@ArgusLi ArgusLi left a comment

Choose a reason for hiding this comment

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

LGTM

@donatobarone
Copy link

is there any reason why this wasn't merged?

@ravjotbrar
Copy link
Contributor Author

@donatobarone It looks like this was missed. Thanks for the reminder!

@ravjotbrar ravjotbrar merged commit 388f540 into main Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArgusLi ArgusLi ArgusLi approved these changes

@jlarue26 jlarue26 Awaiting requested review from jlarue26

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ravjotbrar @ArgusLi @donatobarone