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

Fix a bug where table loader uses nested compute if absent #407

Merged
merged 2 commits into from
Jun 20, 2017
Merged

Fix a bug where table loader uses nested compute if absent #407

merged 2 commits into from
Jun 20, 2017

Conversation

garyluoex
Copy link
Collaborator

No description provided.

@yahoo yahoo deleted a comment Jun 19, 2017
@yahoo yahoo deleted a comment Jun 19, 2017
@yahoo yahoo deleted a comment Jun 19, 2017
@yahoo yahoo deleted a comment Jun 19, 2017
@yahoo yahoo deleted a comment Jun 20, 2017
@yahoo yahoo deleted a comment Jun 20, 2017
@yahoo yahoo deleted a comment Jun 20, 2017
@yahoo yahoo deleted a comment Jun 20, 2017
QubitPi
QubitPi approved these changes Jun 20, 2017
View reviewed changes
Copy link
Contributor

@QubitPi QubitPi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@michael-mclawhorn Not sure if we should make it more readable by iteratively building physical table instead of recursively.

@QubitPi QubitPi self-assigned this Jun 20, 2017
@michael-mclawhorn
Copy link
Contributor

This makes a lot of sense to me.

@michael-mclawhorn
Copy link
Contributor

There should be tests around BaseTableLoader#buildPhysicalTableWithDependency.
I don't insist on them being added on this PR since it isn't changing, but I think we should make an issue to come back and add tests that might have caught this bug.

michael-mclawhorn
michael-mclawhorn approved these changes Jun 20, 2017
View reviewed changes
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

Approved on the condition of a tech debt issue to test this code.

@garyluoex
Copy link
Collaborator Author

@michael-mclawhorn technically the buildDimensionSpanningTableGroup tests the build dependency capability already, but we can discuss if anything is missing.

@garyluoex
Copy link
Collaborator Author

@QubitPi dependency cause building tables iteratively inefficient since it requires several pass to build everything, let me know if you know a way to get around that.

@yahoo yahoo deleted a comment Jun 20, 2017
@garyluoex garyluoex merged commit 00037d1 into master Jun 20, 2017
@garyluoex garyluoex deleted the FixTableLoader branch June 20, 2017 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-mclawhorn michael-mclawhorn michael-mclawhorn approved these changes

@QubitPi QubitPi QubitPi approved these changes

Assignees

@QubitPi QubitPi

Labels
NEED 2 REVIEWS REVIEWABLE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garyluoex @michael-mclawhorn @QubitPi