ARROW-11673 - [C++] Casting dictionary type to use different index type #10721

nirandaperera · 2021-07-14T22:01:36Z

This PR adds casting from one dictionary type to anther dictionary type for both scalars and arrays :
ex:

dictionary(int8(), int16()) --> dictionary(int32(), int64())

github-actions · 2021-07-14T22:01:54Z

https://issues.apache.org/jira/browse/ARROW-11673

cpp/src/arrow/compute/kernels/scalar_cast_dictionary.cc

cpp/src/arrow/compute/kernels/scalar_cast_test.cc

cpp/src/arrow/compute/kernels/scalar_cast_dictionary.cc

cpp/src/arrow/CMakeLists.txt

lidavidm · 2021-07-15T14:05:15Z

cpp/src/arrow/compute/kernels/scalar_cast_test.cc

+  int64_t len = 1000;
+  auto val_arr = rand.ArrayOf(int32(), len, /*null_probability=*/0.01);
+  ASSERT_OK_AND_ASSIGN(auto arr2, DictionaryEncode(val_arr));
+  // check unsafe indices. Cannot validate this array because ValidateOutput throws an


Sorry - now that I look at the JIRA again, it seems having unsafe casts isn't useful for this case? What do you think? If it produces invalid output (presumably, negative and out of bounds indices?), then it seems kind of pointless.

I was also thinking about this. OTOH I was thinking whether the ValidataOutput should check the validity of dictionary array in DictionaryType. 🤔

It looks like it does - are you saying it shouldn't?

I dont think ValidateOutput checks the validity of ArrayData::dictionary field. That's why ValidateOutput passes for unsafe valies AFAIU. :thin

It looks like it does:

arrow/cpp/src/arrow/array/validate.cc

Lines 565 to 572 in dbeed52

Status Visit(const DictionaryType& type) {

const Status indices_status =

CheckBounds(*type.index_type(), 0, data.dictionary->length - 1);

if (!indices_status.ok()) {

return Status::Invalid("Dictionary indices invalid: ", indices_status.ToString());

}

return ValidateArrayFull(*data.dictionary);

}

I think there's no way a primitive array to be validated once the casting is complete (unsafely in this instance).

We're casting from dictionary to dictionary here though.

Anyways, the point stands: this is an unsafe cast that generates an invalid array (and will mostly always do so), so is this case worth supporting?

I think I agree with you. It's not worth testing that. I will remove it. :-)

nirandaperera · 2021-07-15T15:45:48Z

@lidavidm is this a known test failure? [ FAILED ] Expression.ReplaceFieldsWithKnownValues
https://github.com/apache/arrow/pull/10721/checks?check_run_id=3078144913

lidavidm · 2021-07-15T15:51:03Z

Expected 'ReplaceFieldsWithKnownValues( KnownFieldValues{{{"dict_str", dict_i32}}}, expr)' to fail with NotImplemented, but got OK

Looks like this enables something that didn't work before?

  ASSERT_OK_AND_ASSIGN(auto expr, field_ref("dict_str").Bind(*kBoringSchema));
  Datum dict_i32{
      DictionaryScalar::Make(MakeScalar<int32_t>(0), ArrayFromJSON(int32(), R"([3])"))};
  // Unsupported cast dictionary(int32(), int32()) -> dictionary(int32(), utf8())
  ASSERT_RAISES(NotImplemented, ReplaceFieldsWithKnownValues(
                                    KnownFieldValues{{{"dict_str", dict_i32}}}, expr));
  // Unsupported cast dictionary(int8(), utf8()) -> dictionary(int32(), utf8())
  dict_str = Datum{
      DictionaryScalar::Make(MakeScalar<int8_t>(0), ArrayFromJSON(utf8(), R"(["a"])"))};
  ASSERT_RAISES(NotImplemented, ReplaceFieldsWithKnownValues(
                                    KnownFieldValues{{{"dict_str", dict_str}}}, expr));

nirandaperera · 2021-07-15T16:07:59Z

Expected 'ReplaceFieldsWithKnownValues( KnownFieldValues{{{"dict_str", dict_i32}}}, expr)' to fail with NotImplemented, but got OK

Looks like this enables something that didn't work before?

  ASSERT_OK_AND_ASSIGN(auto expr, field_ref("dict_str").Bind(*kBoringSchema));
  Datum dict_i32{
      DictionaryScalar::Make(MakeScalar<int32_t>(0), ArrayFromJSON(int32(), R"([3])"))};
  // Unsupported cast dictionary(int32(), int32()) -> dictionary(int32(), utf8())
  ASSERT_RAISES(NotImplemented, ReplaceFieldsWithKnownValues(
                                    KnownFieldValues{{{"dict_str", dict_i32}}}, expr));
  // Unsupported cast dictionary(int8(), utf8()) -> dictionary(int32(), utf8())
  dict_str = Datum{
      DictionaryScalar::Make(MakeScalar<int8_t>(0), ArrayFromJSON(utf8(), R"(["a"])"))};
  ASSERT_RAISES(NotImplemented, ReplaceFieldsWithKnownValues(
                                    KnownFieldValues{{{"dict_str", dict_str}}}, expr));

Ah! indeed! thanks...

cpp/src/arrow/compute/exec/expression_test.cc

lidavidm · 2021-07-15T20:18:25Z

I think this is good now, thanks! (Just one nit about an unused variable)

lidavidm · 2021-07-15T20:18:38Z

~~I take that back, it's used after all.~~ I had local changes, there's an unused variable 🙂

lidavidm

Thanks for taking this.

edponce · 2021-07-16T20:10:10Z

cpp/src/arrow/compute/kernels/scalar_cast_test.cc

+  check_cast(dictionary(int8(), int16()), dictionary(int8(), int16()),
+             "[1, 2, 3, 1, null, 3]");
+  check_cast(dictionary(int8(), int16()), dictionary(int32(), int64()),
+             "[1, 2, 3, 1, null, 3]");
+  check_cast(dictionary(int32(), utf8()), dictionary(int8(), utf8()),
+             R"(["a", "b", "a", null])");
+


Add tests casting from int to/from float and signed to/from unsigned integers.

I ran int to/from float and unsigned (with Safe and Unsafe options) and it works correctly. LGTM.

lidavidm · 2021-07-19T12:28:31Z

cpp/src/arrow/compute/kernels/scalar_cast_test.cc

@@ -1930,29 +1930,23 @@ TEST(Cast, DictTypeToAnotherDict) {
  check_cast(dictionary(int32(), utf8()), dictionary(int8(), utf8()),
             R"(["a", "b", "a", null])");

+  // check float types (NOTE: ArrayFromJSON doesnt work for float value dictionary types)


What was the error here? Would be nice to file a JIRA for it

Done! ARROW-13381

The error is from ArrayFromJSON(dictionary(..., floatXX()), ...) is:

NotImplemented: JSON conversion to dictionary<values=float, indices=int8, ordered=0> not implemented

Nevertheless, you can Cast successfully to float values with

auto arr = ArrayFromJSON(dictionary(int8(), int32()), "[1, 2, 3, 1, null, 3]"); ASSERT_OK_AND_ASSIGN(auto casted, Cast(arr, dictionary(int8(), float32()), CastOptions::Safe()));

There is also a DictArrayFromJSON but this requires explicit index values:

auto arr = DictArrayFromJSON(dictionary(int8(), float32()), "[0, 1, 2, 3, 4, 5]", "[1, 2, 3, 1, null, 3]");

@nirandaperera I submitted this PR that enables using ArrayFromJSON for dictionaries with floating-point values. If PR passes and gets merged, you can change test accordingly and remove error comment.

lidavidm · 2021-07-19T15:08:19Z

cpp/src/arrow/compute/kernels/scalar_cast_test.cc

@@ -1930,7 +1930,7 @@ TEST(Cast, DictTypeToAnotherDict) {
  check_cast(dictionary(int32(), utf8()), dictionary(int8(), utf8()),
             R"(["a", "b", "a", null])");

-  // check float types (NOTE: ArrayFromJSON doesnt work for float value dictionary types)
+  // check float types (TODO: ARROW-13381 ArrayFromJSON doesnt work for float value dictionary types)


Suggested change

// check float types (TODO: ARROW-13381 ArrayFromJSON doesnt work for float value dictionary types)

// check float types

// TODO(ARROW-13381): ArrayFromJSON doesnt work for float value dictionary types

Needs to be linted.

nirandaperera · 2021-07-19T15:55:48Z

I made the suggested changes and I think this is ready now

kszucs

Thanks Niranda! Merging on green.

github-actions bot added the Component: C++ label Jul 14, 2021

lidavidm reviewed Jul 15, 2021

View reviewed changes

lidavidm mentioned this pull request Jul 15, 2021

ARROW-11673: [C++] Casting dictionary type to use different index type [WIP] #10330

Closed

lidavidm reviewed Jul 15, 2021

View reviewed changes

cpp/src/arrow/compute/exec/expression_test.cc Outdated Show resolved Hide resolved

lidavidm approved these changes Jul 15, 2021

View reviewed changes

edponce suggested changes Jul 16, 2021

View reviewed changes

edponce approved these changes Jul 16, 2021

View reviewed changes

lidavidm approved these changes Jul 19, 2021

View reviewed changes

lidavidm reviewed Jul 19, 2021

View reviewed changes

nirandaperera added 13 commits July 19, 2021 17:15

add initial impl

523c3b8

fixing non-int index_type

d450567

cast only when types differ

35ab8e8

extending test cases

110e0d8

fixing null scalar issue

0a6255c

adding PR comments

37a9bfe

lint fix

ee9ddb3

test fix

4d1a26b

removing unused var

3f10dba

adding float test case

ec58508

updating with JIRA

45c26a6

lint fix

93c018d

rebasing and removing todo

26ffdb3

nirandaperera force-pushed the ARROW-11673 branch from bcc0a02 to 26ffdb3 Compare July 19, 2021 21:22

Python test case

1bad25f

github-actions bot added the Component: Python label Jul 20, 2021

kszucs approved these changes Jul 20, 2021

View reviewed changes

kszucs closed this in a47deef Jul 20, 2021

asfimport mentioned this pull request Jul 20, 2021

[C++] Casting dictionary type to use different index type #27536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-11673 - [C++] Casting dictionary type to use different index type #10721

ARROW-11673 - [C++] Casting dictionary type to use different index type #10721

nirandaperera commented Jul 14, 2021 •

edited

Loading

github-actions bot commented Jul 14, 2021

lidavidm Jul 15, 2021

nirandaperera Jul 15, 2021

lidavidm Jul 15, 2021

nirandaperera Jul 15, 2021

lidavidm Jul 15, 2021

nirandaperera Jul 15, 2021

lidavidm Jul 16, 2021 •

edited

Loading

nirandaperera Jul 18, 2021

nirandaperera commented Jul 15, 2021

lidavidm commented Jul 15, 2021

nirandaperera commented Jul 15, 2021

lidavidm commented Jul 15, 2021

lidavidm commented Jul 15, 2021 •

edited

Loading

lidavidm left a comment

edponce Jul 16, 2021

edponce Jul 16, 2021

lidavidm Jul 19, 2021

nirandaperera Jul 19, 2021 •

edited

Loading

edponce Jul 19, 2021 •

edited

Loading

edponce Jul 19, 2021

lidavidm Jul 19, 2021

lidavidm Jul 19, 2021

nirandaperera commented Jul 19, 2021

kszucs left a comment

	Status Visit(const DictionaryType& type) {
	const Status indices_status =
	CheckBounds(*type.index_type(), 0, data.dictionary->length - 1);
	if (!indices_status.ok()) {
	return Status::Invalid("Dictionary indices invalid: ", indices_status.ToString());
	}
	return ValidateArrayFull(*data.dictionary);
	}

	// check float types (TODO: ARROW-13381 ArrayFromJSON doesnt work for float value dictionary types)
	// check float types
	// TODO(ARROW-13381): ArrayFromJSON doesnt work for float value dictionary types

ARROW-11673 - [C++] Casting dictionary type to use different index type #10721

ARROW-11673 - [C++] Casting dictionary type to use different index type #10721

Conversation

nirandaperera commented Jul 14, 2021 • edited Loading

github-actions bot commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirandaperera commented Jul 15, 2021

lidavidm commented Jul 15, 2021

nirandaperera commented Jul 15, 2021

lidavidm commented Jul 15, 2021

lidavidm commented Jul 15, 2021 • edited Loading

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirandaperera Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

edponce Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirandaperera commented Jul 19, 2021

kszucs left a comment

Choose a reason for hiding this comment

nirandaperera commented Jul 14, 2021 •

edited

Loading

lidavidm Jul 16, 2021 •

edited

Loading

lidavidm commented Jul 15, 2021 •

edited

Loading

nirandaperera Jul 19, 2021 •

edited

Loading

edponce Jul 19, 2021 •

edited

Loading