GH-49634: [Ruby][Integration] Follow dictionary array API change#49635
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
Updates the Ruby Arrow integration JSON reader to follow the dictionary-array API change introduced in GH-49621, so dictionary-encoded columns are constructed with the expected dictionary wrapper type.
Changes:
- Wrap the dictionary value array returned from
read_dictionaryin anArrowFormat::Dictionaryobject. - Pass the wrapped dictionary object into
DictionaryType#build_arrayas the dictionaries collection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| validity_buffer = read_bitmap(column["VALIDITY"]) | ||
| indices_buffer = read_values(column["DATA"], type.index_type) | ||
| dictionary = read_dictionary(type.id, type.value_type) | ||
| dictionary_array = read_dictionary(type.id, type.value_type) |
There was a problem hiding this comment.
read_dictionary can return nil when the dictionary id isn't present in the JSON. Wrapping that in Dictionary.new(type.id, dictionary_array) will defer the failure to a later NoMethodError (e.g., when DictionaryArray#to_a calls dictionary.array). Consider raising a clear exception here if dictionary_array is nil (include the dictionary id and value type) to fail fast with an actionable error.
| dictionary_array = read_dictionary(type.id, type.value_type) | |
| dictionary_array = read_dictionary(type.id, type.value_type) | |
| if dictionary_array.nil? | |
| raise "Dictionary with id #{type.id} and value type " \ | |
| "#{type.value_type.inspect} is missing from JSON dictionaries" | |
| end |
|
+1 Integration test passes again. |
|
After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit 3c25e69. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
|
After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit 3c25e69. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
apache#49635) ### Rationale for this change apacheGH-49621 changed dictionary array related API but we didn't follow the API change in integration test code. ### What changes are included in this PR? Follow dictionary array related API change in integration test code. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#49634 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
apache#49635) ### Rationale for this change apacheGH-49621 changed dictionary array related API but we didn't follow the API change in integration test code. ### What changes are included in this PR? Follow dictionary array related API change in integration test code. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#49634 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
GH-49621 changed dictionary array related API but we didn't follow the API change in integration test code.
What changes are included in this PR?
Follow dictionary array related API change in integration test code.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.