Skip to content

Clean up unused sub-table#3039

Merged
beeender merged 1 commit intomasterfrom
mc/cleanup-subtable
Jun 21, 2016
Merged

Clean up unused sub-table#3039
beeender merged 1 commit intomasterfrom
mc/cleanup-subtable

Conversation

@beeender
Copy link
Copy Markdown
Contributor

@beeender beeender commented Jun 21, 2016

sub-table is not used in java-binding right now, and if it is going to be, the relevant logic might be more suitable in the object store.

Since we are moving to the object store, the sub-table related code needs to be cleaned anyway. So we do the clean up directly to the master branch to make the future PR easier to be reviewed.


Query query = TV(nativeViewPtr)->get_parent().where(TV(nativeViewPtr));
TableQuery* queryPtr = new TableQuery(query);
Query *queryPtr = new Query(std::move(TV(nativeViewPtr)->get_parent().where(TV(nativeViewPtr))));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason for changing this from a TableQuery to Query?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TableQuery is created only for JNI and to wrap around the subtable. And it is deleted in this PR as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, TableQuery was also introduced so we could do a deep copy of the query. @beeender are you certain that the core's Query constructor does a deep copy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is calling move constructor and confirmed with core, it should work 🎉

@cmelchior
Copy link
Copy Markdown
Contributor

Looks 👍
But why the change from TableQuery to Query?

@kneth
Copy link
Copy Markdown
Contributor

kneth commented Jun 21, 2016

@cmelchior TableQuery was partly introduced to keep track of subtable queries (balancing begin and end markers).

@stk1m1
Copy link
Copy Markdown
Contributor

stk1m1 commented Jun 21, 2016

Other than what @kneth pointed, 👍

@bmunkholm
Copy link
Copy Markdown
Contributor

@beeender Please include motivation in the description besides just the title.
It's not clear that it has been confirmed that we are not going to need subtables for any of the other future datatypes we want to support.

@beeender
Copy link
Copy Markdown
Contributor Author

@bmunkholm PR message updated.

@beeender beeender merged commit 76c6178 into master Jun 21, 2016
@beeender beeender deleted the mc/cleanup-subtable branch June 21, 2016 12:39
@beeender beeender removed the S:Review label Jun 21, 2016
LinkViewRef lvr = *lv;
Query query = lvr->get_target_table().where(LinkViewRef(lvr));
TableQuery* queryPtr = new TableQuery(query);
Query *queryPtr = new Query(std::move(lvr->get_target_table().where(LinkViewRef(lvr))));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The use of std::move here is unnecessary since lvr->get_target_table().where(LinkViewRef(lvd)) is already an rvalue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants