-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Remove TableLayout from Accumulo connector #823
Remove TableLayout from Accumulo connector #823
Conversation
bdff503
to
f023b4e
Compare
handle.getTable(), | ||
handle.getRowId(), | ||
newDomain, | ||
getColumnConstraints(handle.getRowId(), newDomain), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to hold on to the column constraints in the table handle? It seems like we should be able to do this directly where they are needed (i.e., in getSplits
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Column constraints are also similar to TupleDomain
which would remain same for all splits so thought of moving it to the AccumuloTableHandle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, they are only used in getSplits()
to call client.getTabletSplits(...)
, but they are not stored in splits. So we could derive them directly in getSplits()
from the information in the handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was passed to AccumuloRecordSet
and AccumuloRecordCursor
but it is not used anywhere expect in getting the split metadata. So we can maintain it as TupleDomain
and derive it from table handle.
f023b4e
to
6b38abc
Compare
6b38abc
to
f86bcb7
Compare
@martint Have removed the column constraint from the table handle. Please let me know if there is any more changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'll merge it once Travis passes.
Removes the TableLayout for Accumulo connector. (#781)