More types in HivePartitionFunction #1466

usurai · 2022-04-22T13:24:49Z

Add support for more types in HivePartitionFunction:

TINYINT
SMALLINT
INTEGER
REAL
DOUBLE
VARBINARY
TIMESTAMP
DATE

Fixes #327

mbasmanova · 2022-05-11T18:06:04Z

@usurai Somehow I missed this. Will take a look soon. In the meantime, would you rebase and update to clear "This branch has conflicts that must be resolved" message?

mbasmanova

@usurai Nice change. Some comments and a question below.

velox/connectors/hive/tests/HivePartitionFunctionTest.cpp

mbasmanova · 2022-05-11T18:08:22Z

velox/connectors/hive/tests/HivePartitionFunctionTest.cpp

+}
+
+TEST_F(HivePartitionFunctionTest, Timestamp) {
+  // TODO Fix flatVectorNullable to set Timestamp.


Would you like to take care of this TODO in a separate PR?

Sure, will do later.

Hi @mbasmanova, seems #778 has already done fixing flatVectorNullable, I change the test of varchar and timestamp to use the initalizer_list form of the API and it works. I created a PR on this. Thanks.

mbasmanova · 2022-05-11T18:10:01Z

velox/connectors/hive/HivePartitionFunction.cpp

+  static_assert(sizeof(float) == sizeof(uint32_t));
+  auto f = [](float value) {
+    uint32_t ret;
+    memcpy(&ret, &value, sizeof ret);


Is it necessary to copy data? Can you use reinterpret_cast instead?

In fact I did some searching before doing this change. Seems although reinterpret_cast the reference / pointer of float to uint32_t works, reading the result of it is undefined behavior.
We might be able to use std::bit_cast when velox supports using C++20.

Reference:

https://stackoverflow.com/questions/53995657/is-reinterpret-cast-type-punning-actually-undefined-behavior

https://timsong-cpp.github.io/cppwp/n4659/basic.lval#8.1

https://stackoverflow.com/questions/48803363/bitwise-casting-uint32-t-to-float-in-c-c

Would you explain a bit more? memcpy is just copying byte-by-byte, it is not type aware, right? Hence, the result would be the same as reinterpret_cast, no?

Would you explain a bit more?

float f = 42.0; uint32_t u = *reinterpret_cast<uint32_t*>(&f);

In practice, this works in some condition[1]. Underlying, reinterpret_cast<uint32_t*>(&f) returns a pointer of type uint32_t that actually points to an object of type float, this is called type punning. As both types are of the same size, we get the exact same result we as do a bit-wise copy.
The issue here is that C++ standard defines "accessing a variable of a given type through a pointer to another type" as undefined behavior, which breaks strict aliasing rule. That is, if type punning is allowed for all the types(not limited in uint32_t and float), some vital optimize will be prohibited. (I'm not expert in this, so I just accept it as a truth)

memcpy is just copying byte-by-byte, it is not type aware, right? Hence, the result would be the same as reinterpret_cast, no?

Yes, they both emit the same result. But the difference is by memcpy, we copy the actually data byte-by-byte to an object of type uint32_t, then when reading it, we are reading an object of type uint32_t. This is legal.

Anyway, I'm totally open to using reinterpret_cast instead of memcpy as long velox's compile flags don't trigger the warning / error of strict-aliasing, and memcpy indeed introduces one more copy which has impact to performance.

[1] To see when it's not working, please compile

#include <iostream> int main() { float f = 42.1; uint32_t u = *reinterpret_cast<uint32_t*>(&f); std::cout << u << std::endl; return 0; }

with

g++ -O2 -Wall -Werror

or more specifically

g++ -O2 -Werror -Wstrict-aliasing

you will see the error

error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] 6 | uint32_t u = *reinterpret_cast<uint32_t*>(&f); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@usurai Thank you for explaining. @laithsakka Laith, would you take a look? Is there a way to avoid a copy here?

@usurai Data copy is expensive, especially one value at a time. I wonder if we could use values.valueAt<uint32_t>() instead.

@usurai Data copy is expensive, especially one value at a time. I wonder if we could use values.valueAt<uint32_t>() instead.

Oh this is optimal. Pushed new change according to this. Thanks!

usurai · 2022-05-11T19:37:57Z

@usurai Somehow I missed this. Will take a look soon. In the meantime, would you rebase and update to clear "This branch has conflicts that must be resolved" message?

Hi @mbasmanova, the merge conflict has been resolved. Thanks.

mbasmanova

Looks good % a couple of minor comments.

velox/connectors/hive/HivePartitionFunction.cpp

mbasmanova · 2022-05-12T12:29:37Z

velox/connectors/hive/HivePartitionFunction.cpp

    const DecodedVector& values,
    vector_size_t size,
    bool mix,
-    std::vector<uint32_t>& hashes) {
+    std::vector<uint32_t>& hashes,
+    std::function<uint32_t(const T&)> const& f) {


naming: f -> hashOne or something similar
order of parameters: input, followed by in/out, followed by output

It might be more efficient to templatize on the function type. It would be nice to add a benchmark in a follow-up PR.

template <typename T, typename THash> void abstractHashTyped( const DecodedVector& values, vector_size_t size, bool mix, THash hashOne, std::vector<uint32_t>& hashes)

naming: f -> hashOne or something similar order of parameters: input, followed by in/out, followed by output

Done renaming function and changing the order.

It might be more efficient to templatize on the function type. It would be nice to add a benchmark in a follow-up PR.

Will do. Should I open an issue to track the progress of benchmark?

Should I open an issue to track the progress of benchmark?

Sure.

I've created a PR adding the benchmark, thanks.

velox/connectors/hive/HivePartitionFunction.cpp

mbasmanova

@usurai Thank you for the contribution.

facebook-github-bot · 2022-05-12T13:06:37Z

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

velox/connectors/hive/tests/HivePartitionFunctionTest.cpp

velox/connectors/hive/HivePartitionFunction.cpp

facebook-github-bot · 2022-05-12T16:05:59Z

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

Summary: Add support for more types in `HivePartitionFunction`: - `TINYINT` - `SMALLINT` - `INTEGER` - `REAL` - `DOUBLE` - `VARBINARY` - `TIMESTAMP` - `DATE` Fixes facebookincubator#327 Pull Request resolved: facebookincubator#1466 Reviewed By: Yuhta Differential Revision: D36345326 Pulled By: mbasmanova fbshipit-source-id: a32b76e8608cac3a8044ef0d468a2c479f62037d

Summary: - Use initialize_list form of `flatVectorNullable` to test `varchar` and `timestamp` and remove the TODO (see #778). - Add `assertPartitionsWithConstChannel` for newly added types. This is a follow-up for #1466 Pull Request resolved: #1625 Reviewed By: kagamiori Differential Revision: D36412294 Pulled By: mbasmanova fbshipit-source-id: 608dda88833df6c16fb53a9009bf2c6269f64c32

Summary: Add support for more types in `HivePartitionFunction`: - `TINYINT` - `SMALLINT` - `INTEGER` - `REAL` - `DOUBLE` - `VARBINARY` - `TIMESTAMP` - `DATE` Fixes facebookincubator#327 Pull Request resolved: facebookincubator#1466 Reviewed By: Yuhta Differential Revision: D36345326 Pulled By: mbasmanova fbshipit-source-id: a32b76e8608cac3a8044ef0d468a2c479f62037d

Summary: - Use initialize_list form of `flatVectorNullable` to test `varchar` and `timestamp` and remove the TODO (see facebookincubator#778). - Add `assertPartitionsWithConstChannel` for newly added types. This is a follow-up for facebookincubator#1466 Pull Request resolved: facebookincubator#1625 Reviewed By: kagamiori Differential Revision: D36412294 Pulled By: mbasmanova fbshipit-source-id: 608dda88833df6c16fb53a9009bf2c6269f64c32

Summary: Add support for more types in `HivePartitionFunction`: - `TINYINT` - `SMALLINT` - `INTEGER` - `REAL` - `DOUBLE` - `VARBINARY` - `TIMESTAMP` - `DATE` Fixes facebookincubator#327 Pull Request resolved: facebookincubator#1466 Reviewed By: Yuhta Differential Revision: D36345326 Pulled By: mbasmanova fbshipit-source-id: a32b76e8608cac3a8044ef0d468a2c479f62037d

Summary: - Use initialize_list form of `flatVectorNullable` to test `varchar` and `timestamp` and remove the TODO (see facebookincubator#778). - Add `assertPartitionsWithConstChannel` for newly added types. This is a follow-up for facebookincubator#1466 Pull Request resolved: facebookincubator#1625 Reviewed By: kagamiori Differential Revision: D36412294 Pulled By: mbasmanova fbshipit-source-id: 608dda88833df6c16fb53a9009bf2c6269f64c32

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 Apr 22, 2022

mbasmanova requested review from mbasmanova, kagamiori, Yuhta and spershin May 11, 2022 18:04

usurai mentioned this pull request May 11, 2022

Add support for more types in HivePartitionFunction #327

Closed

mbasmanova reviewed May 11, 2022

View reviewed changes

usurai added 8 commits May 12, 2022 02:36

hash for TINYINT and test

c6d01c8

add rest types

7012514

format-fix

46306e9

consolidate

a8bca6e

remove redundent

da72378

format-fix

39bb471

remove header

66f5cbe

Rename tests

b014d31

usurai force-pushed the hive_pf_types branch from db54dfa to b014d31 Compare May 11, 2022 19:29

Make REAL & DOUBLE to use logic of INTEGER/BIGINT

a081ee6

mbasmanova reviewed May 12, 2022

View reviewed changes

usurai added 2 commits May 12, 2022 20:59

Change as suggested

cc2e58e

Format fix

c1c07a1

mbasmanova requested a review from majetideepak May 12, 2022 13:03

mbasmanova reviewed May 12, 2022

View reviewed changes

velox/connectors/hive/HivePartitionFunction.cpp Outdated Show resolved Hide resolved

Remove debug header

3392149

mbasmanova approved these changes May 12, 2022

View reviewed changes

Yuhta reviewed May 12, 2022

View reviewed changes

velox/connectors/hive/tests/HivePartitionFunctionTest.cpp Outdated Show resolved Hide resolved

velox/connectors/hive/tests/HivePartitionFunctionTest.cpp Outdated Show resolved Hide resolved

laithsakka requested changes May 12, 2022

View reviewed changes

velox/connectors/hive/HivePartitionFunction.cpp Outdated Show resolved Hide resolved

Replace min with lowest for real & double

7a4328c

Templatize function

525d23f

usurai requested a review from laithsakka May 12, 2022 16:17

facebook-github-bot closed this in ccd39e4 May 12, 2022

This was referenced May 15, 2022

Enhance HivePartitionFunctionTest #1625

Closed

Benchmark HivePartitionFunction #1626

Closed

usurai deleted the hive_pf_types branch May 31, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More types in HivePartitionFunction #1466

More types in HivePartitionFunction #1466

usurai commented Apr 22, 2022 •

edited by mbasmanova

Loading

mbasmanova commented May 11, 2022

mbasmanova left a comment

mbasmanova May 11, 2022

usurai May 11, 2022

usurai May 15, 2022

mbasmanova May 11, 2022

usurai May 11, 2022

mbasmanova May 11, 2022

usurai May 12, 2022 •

edited

Loading

mbasmanova May 12, 2022

mbasmanova May 12, 2022

usurai May 12, 2022

usurai commented May 11, 2022

mbasmanova left a comment

mbasmanova May 12, 2022

usurai May 12, 2022

mbasmanova May 12, 2022

usurai May 17, 2022

mbasmanova left a comment

facebook-github-bot commented May 12, 2022

facebook-github-bot commented May 12, 2022

More types in HivePartitionFunction #1466

More types in HivePartitionFunction #1466

Conversation

usurai commented Apr 22, 2022 • edited by mbasmanova Loading

mbasmanova commented May 11, 2022

mbasmanova 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usurai May 12, 2022 • 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

usurai commented May 11, 2022

mbasmanova 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

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 12, 2022

facebook-github-bot commented May 12, 2022

usurai commented Apr 22, 2022 •

edited by mbasmanova

Loading

usurai May 12, 2022 •

edited

Loading