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.

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

fix(query): override RemoteExec::doExecute instead of execute() #1490

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

alextheimer
Copy link
Contributor

@alextheimer alextheimer commented Dec 9, 2022

Pull Request checklist

  • The commit(s) message(s) follows the contribution guidelines ?
  • Tests for the changes have been added (for bug fixes / features) ?
  • Docs have been added / updated (for bug fixes / features) ?

Currently, RemoteExec plans override the usual execute logic; this means that common tasks like RangeVectorTransformer application will be missed if they are not explicitly performed in the overridden execute.

This PR moves the overridden logic into doExecute, so all common execute tasks will be performed automatically.

Comment on lines -170 to -176
case QueryResult(id, _, response, _, _, _) => {
val rv = response(0)
rv.rows.size shouldEqual 0
rv.rows.map { row =>
val record = row.asInstanceOf[BinaryRecordRowReader]
rv.asInstanceOf[SerializedRangeVector].schema.toStringPairs(record.recordBase, record.recordOffset)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sketched out by this-- @sherali42 , do we need the result to contain an empty RV?

@amolnayak311
Copy link
Contributor

amolnayak311 commented Dec 9, 2022

What requires this change? Currently since execute is overridden, no RVTs are applied. How do we ensure now that the transformation is not applied twice? Once on remote side and once locally after the response from remote partition is received?

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