Handle ShortDecimal correctly inside `importFromArrow` #8957

boneanxs · 2024-03-05T10:27:20Z

Arrow uses int128_t to store ShortDecimal values, while inside velox we use int64_t.

ExportToArrow already handle it specifically, but ImportFromArrow misses this.

This pr tries to fix it.

facebook-github-bot · 2024-03-05T10:27:28Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

netlify · 2024-03-05T10:27:39Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`02a6ab6`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65fb92648e11560008d1ba6b

boneanxs · 2024-03-06T02:26:30Z

Hey @PHILO-HE @rui-mo @pedroerp, pls review this, thanks!

rui-mo

Thanks for the fix. Just added several comments.

rui-mo · 2024-03-06T03:04:44Z

velox/vector/arrow/Bridge.cpp

+    if (type->isShortDecimal()) {
+      values = AlignedBuffer::allocate<int64_t>(arrowArray.length, pool);
+      auto rawValues = values->asMutable<int64_t>();
+      auto oldValues = static_cast<const int128_t*>(arrowArray.buffers[1]);


It looks like arrow array can hold a decimal of a bit-width other than int128_t (int256 is also possible). Maybe we can add a check in Velox importFromArrowImpl to ensure the bit-width is not set or the bit-width is indeed 128. Then it should be safe to cast buffers[1] as int128* here.
https://github.com/apache/arrow/blob/main/cpp/src/arrow/c/bridge.cc#L348-L356

Since type is firstly called here:

velox/velox/vector/arrow/Bridge.cpp

Line 1443 in 09ba220

auto type = importFromArrow(arrowSchema);

, whereas the decimal type must be int128_t, otherwise importFromArrowImpl could already fail, do we need to add extra check here?

I tried below cases in the unit test TEST_F(ArrowBridgeSchemaImportTest, scalar), and they both work. Could you help confirm?

EXPECT_EQ(*DECIMAL(10, 4), *testSchemaImport("d:10,4,128"));
EXPECT_EQ(*DECIMAL(10, 4), *testSchemaImport("d:10,4,256"));

velox/vector/arrow/Bridge.cpp

rui-mo · 2024-03-06T03:11:03Z

velox/vector/arrow/tests/ArrowBridgeArrayTest.cpp

@@ -1102,6 +1102,21 @@ class ArrowBridgeArrayImportTest : public ArrowBridgeArrayExportTest {
    }
  }

+  void testShortDecimalImport() {


Could we reuse the method testArrowImport? Maybe a template parameter TOuput and a default argument for the output values need to be added.

Hey, @rui-mo I tried to reuse the method but the codes looks lengthy and not elegant... Could you pls give me more hint in case I miss something We have to pass int128_t as arrowInput while int64_t as vector raw values, and take these values as Decimal type...😣

I implemented some code in rui-mo@a2d33ff. Please check if it makes sense, thanks.

rui-mo

Thanks!

velox/vector/arrow/Bridge.cpp

rui-mo · 2024-03-07T11:48:36Z

velox/vector/arrow/Bridge.cpp

+          int bitWidth = std::stoi(&format[idx + sz + 1], &sz);
+          if (bitWidth != 128) {
+            VELOX_USER_FAIL(
+                "Conversion failed for '{}'. Velox decimal does not support custom bitwidth.",


VELOX_USER_CHECK_EQ(bitWidth, 128, "msg");

rui-mo

Thanks. Just two nits.

velox/vector/arrow/Bridge.cpp

velox/vector/arrow/tests/ArrowBridgeArrayTest.cpp

rui-mo

Thanks. Let's see if the @mbasmanova @Yuhta have any suggestion.

boneanxs · 2024-03-12T01:50:38Z

Gentle ping @Yuhta @majetideepak @pedroerp , hey, could you pls help review this?

velox/vector/arrow/Bridge.cpp

velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp

pedroerp · 2024-03-15T15:43:57Z

velox/vector/arrow/Bridge.cpp

@@ -972,7 +972,17 @@ TypePtr importFromArrowImpl(
        // Parse "d:".


for readability, maybe also move the logic to a helper funciton?

case 'd': return parseDecimalFormat(format);

pedroerp

Thank you!

facebook-github-bot · 2024-03-18T15:37:02Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pedroerp · 2024-03-18T16:00:02Z

Like I pointed out above, the test still access out of bounds memory in the string. We got this with ASAN runs on the new tests:

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ArrowBridgeSchemaImportTest
[ RUN      ] ArrowBridgeSchemaImportTest.scalar

=================================================================
==48809==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000307f05 at pc 0x000000ab95f9 bp 0x7ffca1dba5a0 sp 0x7ffca1db9d60
READ of size 1 at 0x000000307f05 thread T0
SCARINESS: 12 (1-byte-read-global-buffer-overflow)
    #0 0xab95f8 in __interceptor_strlen.part.0 ubsan.c
    #1 0x7f407300dafd in std::char_traits<char>::length(char const*) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/char_traits.h:371
    #2 0x7f407300a708 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::basic_string<std::allocator<char>>(char const*, std::allocator<char> const&) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/basic_string.h:536
    #3 0x7f407340994b in facebook::velox::(anonymous namespace)::parseDecimalFormat(char const*) fbcode/velox/vector/arrow/Bridge.cpp:939
    #4 0x7f4073406e4b in facebook::velox::(anonymous namespace)::importFromArrowImpl(char const*, ArrowSchema const&) fbcode/velox/vector/arrow/Bridge.cpp:1005
    #5 0x7f4073405efc in facebook::velox::importFromArrow(ArrowSchema const&) fbcode/velox/vector/arrow/Bridge.cpp:1224
    #6 0xa2fcab in facebook::velox::test::(anonymous namespace)::ArrowBridgeSchemaImportTest::testSchemaImport(char const*) fbcode/velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp:259
    #7 0xa2e85a in facebook::velox::test::(anonymous namespace)::ArrowBridgeSchemaImportTest_scalar_Test::TestBody() fbcode/velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp:381
    #8 0x7f4080a5689e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) fbsource/src/gtest.cc:2675
    #9 0x7f4080a56124 in testing::Test::Run() fbsource/src/gtest.cc:2692

boneanxs · 2024-03-19T10:32:46Z

velox/vector/arrow/Bridge.cpp

+    auto firstCommaIdx = formatStr.find(',', 2);
+    auto secondCommaIdx = formatStr.find(',', firstCommaIdx + 1);
+
+    if (firstCommaIdx == std::string::npos ||


Add extra check here to avoid ASAN issue, cc @pedroerp

facebook-github-bot · 2024-03-20T22:44:30Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-03-21T23:45:05Z

@pedroerp merged this pull request in dc561a3.

conbench-facebook · 2024-03-22T00:08:26Z

Conbench analyzed the 1 benchmark run on commit dc561a35.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ator#8957) Summary: Arrow uses `int128_t` to store ShortDecimal values, while inside velox we use `int64_t`. `ExportToArrow` already handle it specifically, but `ImportFromArrow` misses this. This pr tries to fix it. Pull Request resolved: facebookincubator#8957 Reviewed By: Yuhta Differential Revision: D55019687 Pulled By: pedroerp fbshipit-source-id: 2fe32236a0e17a52ef713cff96836a48a37fec56

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 5, 2024

boneanxs mentioned this pull request Mar 6, 2024

[GLUTEN-4241][VL] Add plan node to convert Vanilla spark columnar format data to Velox columnar format data apache/incubator-gluten#4818

Closed

rui-mo reviewed Mar 6, 2024

View reviewed changes

boneanxs force-pushed the fix_short_decimal branch from 9502797 to ab1d17a Compare March 7, 2024 10:56

boneanxs requested a review from rui-mo March 7, 2024 10:57

rui-mo reviewed Mar 7, 2024

View reviewed changes

boneanxs requested a review from rui-mo March 8, 2024 02:31

rui-mo approved these changes Mar 8, 2024

View reviewed changes

velox/vector/arrow/Bridge.cpp Outdated Show resolved Hide resolved

velox/vector/arrow/tests/ArrowBridgeArrayTest.cpp Show resolved Hide resolved

boneanxs requested a review from rui-mo March 8, 2024 07:03

rui-mo reviewed Mar 8, 2024

View reviewed changes

mbasmanova requested review from Yuhta, majetideepak and pedroerp March 8, 2024 12:35

pedroerp reviewed Mar 12, 2024

View reviewed changes

velox/vector/arrow/Bridge.cpp Outdated Show resolved Hide resolved

pedroerp reviewed Mar 15, 2024

View reviewed changes

velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp Show resolved Hide resolved

pedroerp reviewed Mar 15, 2024

View reviewed changes

boneanxs force-pushed the fix_short_decimal branch from f1830f0 to 958e6b0 Compare March 18, 2024 10:36

boneanxs requested a review from pedroerp March 18, 2024 10:39

pedroerp approved these changes Mar 18, 2024

View reviewed changes

Yuhta approved these changes Mar 18, 2024

View reviewed changes

boneanxs commented Mar 19, 2024

View reviewed changes

boneanxs force-pushed the fix_short_decimal branch from 725b988 to 389a6f9 Compare March 20, 2024 02:41

importFromArrow doesn't handle shortdecimal correctly

b9b3817

boneanxs added 7 commits March 20, 2024 21:28

address comments

168cd15

Address comment

f2f45db

Optimize message

95b073b

Fix comments

7e96120

Address comments

d4ce0ee

Avoid ASAN

1f5ec51

Rebase codes

02a6ab6

boneanxs force-pushed the fix_short_decimal branch from 389a6f9 to 02a6ab6 Compare March 21, 2024 01:50

facebook-github-bot closed this in dc561a3 Mar 21, 2024

facebook-github-bot added the Merged label Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle ShortDecimal correctly inside `importFromArrow` #8957

Handle ShortDecimal correctly inside `importFromArrow` #8957

boneanxs commented Mar 5, 2024

facebook-github-bot commented Mar 5, 2024

netlify bot commented Mar 5, 2024 •

edited

Loading

boneanxs commented Mar 6, 2024

rui-mo left a comment

rui-mo Mar 6, 2024 •

edited

Loading

boneanxs Mar 7, 2024

rui-mo Mar 7, 2024 •

edited

Loading

rui-mo Mar 6, 2024

boneanxs Mar 7, 2024

rui-mo Mar 7, 2024

rui-mo left a comment

rui-mo Mar 7, 2024

rui-mo left a comment

rui-mo left a comment

boneanxs commented Mar 12, 2024

pedroerp Mar 15, 2024

pedroerp left a comment

facebook-github-bot commented Mar 18, 2024

pedroerp commented Mar 18, 2024

boneanxs Mar 19, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 21, 2024

conbench-facebook bot commented Mar 22, 2024

		@@ -972,7 +972,17 @@ TypePtr importFromArrowImpl(
		// Parse "d:".

Handle ShortDecimal correctly inside importFromArrow #8957

Handle ShortDecimal correctly inside importFromArrow #8957

Conversation

boneanxs commented Mar 5, 2024

facebook-github-bot commented Mar 5, 2024

Action Required

Process

netlify bot commented Mar 5, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

boneanxs commented Mar 6, 2024

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

boneanxs Mar 7, 2024

Choose a reason for hiding this comment

rui-mo Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

rui-mo Mar 6, 2024

Choose a reason for hiding this comment

boneanxs Mar 7, 2024

Choose a reason for hiding this comment

rui-mo Mar 7, 2024

Choose a reason for hiding this comment

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Mar 7, 2024

Choose a reason for hiding this comment

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo left a comment

Choose a reason for hiding this comment

boneanxs commented Mar 12, 2024

pedroerp Mar 15, 2024

Choose a reason for hiding this comment

pedroerp left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 18, 2024

pedroerp commented Mar 18, 2024

boneanxs Mar 19, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 21, 2024

conbench-facebook bot commented Mar 22, 2024

Handle ShortDecimal correctly inside `importFromArrow` #8957

Handle ShortDecimal correctly inside `importFromArrow` #8957

netlify bot commented Mar 5, 2024 •

edited

Loading

rui-mo Mar 6, 2024 •

edited

Loading

rui-mo Mar 7, 2024 •

edited

Loading