You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
cpp/src/binaryop/binaryop.cpp has no dictionary handling (git grep -i dictionary cpp/src/binaryop/ finds nothing). For (DICTIONARY32, scalar) and (DICTIONARY32, plain column) that ends in the intended CUDF_FAIL("Unsupported operator for these types").
For (DICTIONARY32, DICTIONARY32) it does not fail. column_device_view::element<dictionary32>() returns the index, and dictionary_wrapper defines ==, !=, <, >, <=, >=, so is_supported_operation() returns true and the compiled kernel compares indices. Indices are only meaningful relative to their own keys column, so the result is a silently wrong answer whenever the two inputs do not agree index-for-index — which is the normal case, since each column is encoded independently.
There are no dictionary tests under cpp/tests/binaryop/.
Steps/Code to reproduce bug
auto a_plain = /* INT32 column: 10, 20, 30, 20 */;
auto b_plain = /* INT32 column: 30, 20, 10, 10 */;
auto a = cudf::dictionary::encode(a_plain->view()); // keys {10,20,30}, indices 0,1,2,1auto b = cudf::dictionary::encode(b_plain->view()); // keys {10,20,30}, indices 2,1,0,0auto r = cudf::binary_operation(a->view(), b->view(),
cudf::binary_operator::EQUAL,
cudf::data_type{cudf::type_id::BOOL8});
Comparing the same two logical value sequences elementwise should give 0 1 0 0.
Measured on 26.08 (libcudf.so from the conda package), single GPU:
a keys=3 indices_type=3
RESULT (expected 0 1 0 0): 1 1 1 0
null_count=0
Every row is wrong except the one where the indices happen to coincide. Note this example is the favourable case — both columns even have identical key sets; with different key sets the answer is arbitrary. No error, no warning.
Expected behavior
Either
Fail loudly (smallest fix): make binops::compiled::is_supported_operation() reject DICTIONARY32 so the call raises like the scalar and mixed cases already do, and add a regression test. This matches how every other unsupported combination behaves today.
Support it properly: match the key sets first, the way search/contains_column.cu does with add_keys/set_keys, then compare. This is the only remaining unchecked item in [FEA] Remaining dictionary column work in libcudf #5963 ("Remaining dictionary column work in libcudf") — unary_operation, reduce, quantile, minmax, contains, clamp, merge, joins, copy_if_else, rolling and groupby all gained dictionary support in the 0.16/0.17 cycle, while binaryop never did.
Option 1 is safe to take immediately and does not preclude option 2.
Environment overview
Environment location: conda (libcudf 26.08)
Method of cuDF install: conda
Additional context
Found while building an execution path that keeps Parquet dictionary columns encoded through the query engine (predicates are evaluated on the keys, so this call is guarded on our side and we are not hitting the wrong results in production). Related dictionary issues we have filed or fixed: #23887 / #23889 (narrow-index concatenate out-of-bounds) and #23888 (pack/unpack silently corrupting dictionary columns).
Describe the bug
cpp/src/binaryop/binaryop.cpphas no dictionary handling (git grep -i dictionary cpp/src/binaryop/finds nothing). For(DICTIONARY32, scalar)and(DICTIONARY32, plain column)that ends in the intendedCUDF_FAIL("Unsupported operator for these types").For
(DICTIONARY32, DICTIONARY32)it does not fail.column_device_view::element<dictionary32>()returns the index, anddictionary_wrapperdefines==,!=,<,>,<=,>=, sois_supported_operation()returns true and the compiled kernel compares indices. Indices are only meaningful relative to their own keys column, so the result is a silently wrong answer whenever the two inputs do not agree index-for-index — which is the normal case, since each column is encoded independently.There are no dictionary tests under
cpp/tests/binaryop/.Steps/Code to reproduce bug
Comparing the same two logical value sequences elementwise should give
0 1 0 0.Measured on 26.08 (
libcudf.sofrom the conda package), single GPU:Every row is wrong except the one where the indices happen to coincide. Note this example is the favourable case — both columns even have identical key sets; with different key sets the answer is arbitrary. No error, no warning.
Expected behavior
Either
binops::compiled::is_supported_operation()rejectDICTIONARY32so the call raises like the scalar and mixed cases already do, and add a regression test. This matches how every other unsupported combination behaves today.search/contains_column.cudoes withadd_keys/set_keys, then compare. This is the only remaining unchecked item in [FEA] Remaining dictionary column work in libcudf #5963 ("Remaining dictionary column work in libcudf") —unary_operation,reduce,quantile,minmax,contains,clamp,merge, joins,copy_if_else, rolling and groupby all gained dictionary support in the 0.16/0.17 cycle, whilebinaryopnever did.Option 1 is safe to take immediately and does not preclude option 2.
Environment overview
libcudf26.08)Additional context
Found while building an execution path that keeps Parquet dictionary columns encoded through the query engine (predicates are evaluated on the keys, so this call is guarded on our side and we are not hitting the wrong results in production). Related dictionary issues we have filed or fixed: #23887 / #23889 (narrow-index
concatenateout-of-bounds) and #23888 (pack/unpacksilently corrupting dictionary columns).