[native] ArrowFlight connector missing ColumnHandle operator<()#24914
Conversation
prestodb-ci
requested review from
a team,
nishithakbhaskaran and
pramodsatya
and removed request for
a team
| {{#comparable}} | ||
| virtual bool operator<(const {{&class_name}}& /* o */) const { | ||
| throw std::runtime_error("missing operator<() in {class_name} subclass"); | ||
| throw std::runtime_error("missing operator<() in {{class_name}} subclass"); |
There was a problem hiding this comment.
This was causing the error message to display as missing operator<() in {class_name} subclass. The double braces are needed to correct it when generating the protocol.
pramodsatya
left a comment
pramodsatya
left a comment
There was a problem hiding this comment.
Thanks @BryanCutler, LGTM.
| }; | ||
| void to_json(json& j, const ArrowColumnHandle& p); | ||
| void from_json(const json& j, ArrowColumnHandle& p); | ||
| } // namespace facebook::presto::protocol::arrow_flight No newline at end of file |
There was a problem hiding this comment.
nit: Add newline at end
There was a problem hiding this comment.
Thanks for catching that @pramodsatya! I added a newline and spacing to match the similar files from the other connectors, then generated the protocol.
BryanCutler
force-pushed
the
arrow-flight-add-native-col-handle-operator
branch
3 times, most recently
from
c55f20f to
562c121
Compare
|
@BryanCutler : The code looks good. Can you combine your commits into a single one with the same titile as this PR ? Will approve then. |
BryanCutler
force-pushed
the
arrow-flight-add-native-col-handle-operator
branch
from
562c121 to
e97eeda
Compare
| void from_json(const json& j, ArrowTransactionHandle& p); | ||
|
|
||
| } // namespace facebook::presto::protocol::arrow_flight | ||
| /* |
There was a problem hiding this comment.
@BryanCutler Its strange this copyright message is inserted here again... It needs to be in the header only. Please can you fix it if possible.
There was a problem hiding this comment.
The special file ArrowColumnHandle.hpp.inc has a copyright header and when the protocol is generated it puts the entire file into presto_protocol_arrow_flight.h. It's the same thing for the Hive connector and all the *.inc files.
I thought it was a bit strange too, but it seems like the *.inc files need a header, so it's just the side effect.
There was a problem hiding this comment.
We should fix this in the scripting. Please create an issue for it.
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @BryanCutler
BryanCutler
force-pushed
the
arrow-flight-add-native-col-handle-operator
branch
from
e97eeda to
3d5f6bd
Compare
ArrowColumnHandle was missing the operator<() in the protocol definition. This was causing queries that require ordering of columns to fail. A special definition of ArrowColumnHandle is required in the protocol to define the operator to order columns by the columnName. Add e2e test for ColumnHandle ordering Add a Java unit test to test the native ArrowFlight connector will support queries that require ordering of column names.
BryanCutler
force-pushed
the
arrow-flight-add-native-col-handle-operator
branch
from
3d5f6bd to
ffe2bb0
Compare
6 participants
Description
ArrowColumnHandle was missing the operator<() in the protocol definition. This was causing queries that require ordering of columns to fail. A special definition of ArrowColumnHandle is required in the protocol to define the operator to order columns by the columnName.
Add a Java unit test to test the native ArrowFlight connector will support queries that require ordering of column names.
Fixed the error message to correctly insert the mustache template for the
ColumnHandleclass.Motivation and Context
Queries using the ArrowFlight connector that require ordering of column names will fail with the error:
Impact
NA
Test Plan
Added a Java e2e with a query that reproduces the error.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.