Fix null value check for kRange frame column #11075
Conversation
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
Hi @pramodsatya, thank you for the fix! It looks mostly good to me. I left a few comments.
1557f96 to
a560206
Compare
There was a problem hiding this comment.
Thanks for the feedback @kagamiori, addressed the comments. Could you please take another look?
a560206 to
ba41a62
Compare
ba41a62 to
bcc9e83
Compare
There was a problem hiding this comment.
Thanks for the feedback @aditi-pandit, addressed the comments. Could you please take another look?
bcc9e83 to
0b9d3d3
Compare
| @@ -487,5 +487,41 @@ TEST_F(WindowTest, nagativeFrameArg) { | |||
| } | |||
| } | |||
|
|
|||
| DEBUG_ONLY_TEST_F(WindowTest, frameColumnNullCheck) { | |||
There was a problem hiding this comment.
Curious why we make it DEBUG_ONLY test?
There was a problem hiding this comment.
@kagamiori : Because the assertion which crashed was a DCHECK. It didn't really participate in any logic which had correct offsets. If we had commented out the DCHECK, the code would've worked correctly.
I feel that code should retain as a DCHECK only. We don't anticipate it at runtime, but is an assumption about how things are setup.
|
@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kagamiori merged this pull request in 4da43a1. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: The columns in `WindowPartition::data_` are reordered to start with partition-by keys and order-by keys, whereas the columns in `WindowPartition::columns_` are in the same order as the Window operator. The variable `orderByColumn` contains the index of order-by key in the reordered columns, so this should be accessed from `data_` and not `columns_`. A unit test is added to ensure an exception is thrown when NULL values in order-by column and frame column do not match. Pull Request resolved: facebookincubator#11075 Reviewed By: amitkdutta Differential Revision: D63404094 Pulled By: kagamiori fbshipit-source-id: cae3713c3c6c945001af8c2930ba08a34d574f67
The columns in
WindowPartition::data_are reordered to start with partition-by keys and order-by keys, whereas the columns inWindowPartition::columns_are in the same order as the Window operator. The variableorderByColumncontains the index of order-by key in the reordered columns, so this should be accessed fromdata_and notcolumns_. A unit test is added to ensure an exception is thrown when NULL values in order-by column and frame column do not match.