From 76b5d0e9267d7608b82c5e66f671f6b2f6685dd1 Mon Sep 17 00:00:00 2001 From: Masha Basmanova Date: Mon, 12 Feb 2024 07:50:40 -0800 Subject: [PATCH 01/38] Remove deprecated ExchangeSource::request(maxBytes) API (#8727) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8727 Reviewed By: Yuhta Differential Revision: D53662612 fbshipit-source-id: c7f6b1e1a2dc9bb26d1db58afb5bf0a69037333c --- velox/exec/ExchangeSource.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/velox/exec/ExchangeSource.h b/velox/exec/ExchangeSource.h index 59a3a339f5d6..8cee990d8c02 100644 --- a/velox/exec/ExchangeSource.h +++ b/velox/exec/ExchangeSource.h @@ -59,14 +59,6 @@ class ExchangeSource : public std::enable_shared_from_this { return requestPending_; } - /// Requests the producer to generate up to 'maxBytes' more data. - /// Returns a future that completes when producer responds either with 'data' - /// or with a message indicating that all data has been already produced or - /// data will take more time to produce. - virtual ContinueFuture request(uint32_t /*maxBytes*/) { - VELOX_NYI(); - } - struct Response { /// Size of the response in bytes. Zero means response didn't contain any /// data. From 7830d641d3a6a69e0dd75d84c3a77a065d5e7d9a Mon Sep 17 00:00:00 2001 From: PHILO-HE Date: Mon, 12 Feb 2024 09:16:35 -0800 Subject: [PATCH 02/38] Support UNKNOWN type in exporting to Arrow array (#8724) Summary: In arrow, "n" is used to denote NullType which is UNKNOWN type in velox. See [arrow code link](https://github.com/apache/arrow/blob/main/cpp/src/arrow/c/bridge.cc#L318). Pull Request resolved: https://github.com/facebookincubator/velox/pull/8724 Reviewed By: Yuhta Differential Revision: D53663645 Pulled By: mbasmanova fbshipit-source-id: 67afdb5027ee5a236be5c22f05c889ddfb0cf9ac --- velox/vector/arrow/Bridge.cpp | 5 +++++ .../arrow/tests/ArrowBridgeSchemaTest.cpp | 21 ++++++------------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/velox/vector/arrow/Bridge.cpp b/velox/vector/arrow/Bridge.cpp index 601cbc08e847..6522ab9f18fc 100644 --- a/velox/vector/arrow/Bridge.cpp +++ b/velox/vector/arrow/Bridge.cpp @@ -248,6 +248,8 @@ const char* exportArrowFormatStr( return "u"; // utf-8 string case TypeKind::VARBINARY: return "z"; // binary + case TypeKind::UNKNOWN: + return "n"; // NullType case TypeKind::TIMESTAMP: return "ttn"; // time64 [nanoseconds] @@ -598,6 +600,7 @@ void exportFlat( case TypeKind::REAL: case TypeKind::DOUBLE: case TypeKind::TIMESTAMP: + case TypeKind::UNKNOWN: exportValues(vec, rows, out, pool, holder); break; case TypeKind::VARCHAR: @@ -940,6 +943,8 @@ TypePtr importFromArrowImpl( return REAL(); case 'g': return DOUBLE(); + case 'n': + return UNKNOWN(); // Map both utf-8 and large utf-8 string to varchar. case 'u': diff --git a/velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp b/velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp index 8def65ce8e8e..a880d93f1a97 100644 --- a/velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp +++ b/velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp @@ -195,6 +195,8 @@ TEST_F(ArrowBridgeSchemaExportTest, scalar) { testScalarType(DECIMAL(10, 4), "d:10,4"); testScalarType(DECIMAL(20, 15), "d:20,15"); + + testScalarType(UNKNOWN(), "n"); } TEST_F(ArrowBridgeSchemaExportTest, nested) { @@ -238,24 +240,14 @@ TEST_F(ArrowBridgeSchemaExportTest, constant) { testConstant(DOUBLE(), "g"); testConstant(VARCHAR(), "u"); testConstant(DATE(), "tdD"); + testConstant(UNKNOWN(), "n"); testConstant(ARRAY(INTEGER()), "+l"); + testConstant(ARRAY(UNKNOWN()), "+l"); testConstant(MAP(BOOLEAN(), REAL()), "+m"); + testConstant(MAP(UNKNOWN(), REAL()), "+m"); testConstant(ROW({TIMESTAMP(), DOUBLE()}), "+s"); -} - -TEST_F(ArrowBridgeSchemaExportTest, unsupported) { - // Try some combination of unsupported types to ensure there's no crash or - // memory leak in failure scenarios. - EXPECT_THROW(testScalarType(UNKNOWN(), ""), VeloxException); - - EXPECT_THROW(testScalarType(ARRAY(UNKNOWN()), ""), VeloxException); - EXPECT_THROW(testScalarType(MAP(UNKNOWN(), INTEGER()), ""), VeloxException); - EXPECT_THROW(testScalarType(MAP(BIGINT(), UNKNOWN()), ""), VeloxException); - - EXPECT_THROW(testScalarType(ROW({BIGINT(), UNKNOWN()}), ""), VeloxException); - EXPECT_THROW( - testScalarType(ROW({BIGINT(), REAL(), UNKNOWN()}), ""), VeloxException); + testConstant(ROW({UNKNOWN(), UNKNOWN()}), "+s"); } class ArrowBridgeSchemaImportTest : public ArrowBridgeSchemaExportTest { @@ -395,7 +387,6 @@ TEST_F(ArrowBridgeSchemaImportTest, complexTypes) { } TEST_F(ArrowBridgeSchemaImportTest, unsupported) { - EXPECT_THROW(testSchemaImport("n"), VeloxUserError); EXPECT_THROW(testSchemaImport("C"), VeloxUserError); EXPECT_THROW(testSchemaImport("S"), VeloxUserError); EXPECT_THROW(testSchemaImport("I"), VeloxUserError); From c51abe36b7fba1142a9110129209b9a80442cbd1 Mon Sep 17 00:00:00 2001 From: Wei He Date: Mon, 12 Feb 2024 10:19:35 -0800 Subject: [PATCH 03/38] Fix incorrect result of first_value/last_value functions (#8626) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8626 The first_value and last_value functions produce incorrect results when used with IGNORE NULLS and when the frame doesn't start from the first row. In this situation, FirstLastValueFunction::setRowNumbersIgnoreNulls() attemps to find the index of the first non-null value from `leastFrame` to `leastFrame + frameSize`. But the index found is relative to `leastFrame`, so setRowNumbersIgnoreNulls() should add leastFrame to the index before returning it. This diff fixes https://github.com/facebookincubator/velox/issues/8427. Reviewed By: kgpai Differential Revision: D53295212 fbshipit-source-id: 614f41b5ced7059aad475dca1967a02ba1c4beec --- .../prestosql/window/FirstLastValue.cpp | 6 +++-- .../prestosql/window/tests/NthValueTest.cpp | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/velox/functions/prestosql/window/FirstLastValue.cpp b/velox/functions/prestosql/window/FirstLastValue.cpp index bab12e5d01cd..eaec4db98703 100644 --- a/velox/functions/prestosql/window/FirstLastValue.cpp +++ b/velox/functions/prestosql/window/FirstLastValue.cpp @@ -127,11 +127,13 @@ class FirstLastValueFunction : public exec::WindowFunction { // The function returns null for this case. -1 correctly maps to // kNullRow as expected for rowNumbers_ extraction. if constexpr (TValue == ValueType::kFirst) { - rowNumbers_[i] = bits::findFirstBit( + auto position = bits::findFirstBit( rawNonNulls, frameStart - leastFrame, frameEnd - leastFrame + 1); + rowNumbers_[i] = (position == -1) ? -1 : position + leastFrame; } else { - rowNumbers_[i] = bits::findLastBit( + auto position = bits::findLastBit( rawNonNulls, frameStart - leastFrame, frameEnd - leastFrame + 1); + rowNumbers_[i] = (position == -1) ? -1 : position + leastFrame; } }); } diff --git a/velox/functions/prestosql/window/tests/NthValueTest.cpp b/velox/functions/prestosql/window/tests/NthValueTest.cpp index 18715361c8fa..c27c690fabcc 100644 --- a/velox/functions/prestosql/window/tests/NthValueTest.cpp +++ b/velox/functions/prestosql/window/tests/NthValueTest.cpp @@ -335,6 +335,32 @@ TEST_F(NthValueTest, ignoreNulls) { } } +TEST_F(NthValueTest, frameStartsFromFollowing) { + auto input = makeRowVector({ + makeNullableFlatVector({1, std::nullopt, 2}), + makeFlatVector({false, false, false}), + makeFlatVector({1, 2, 3}), + }); + auto expected = makeRowVector( + {makeNullableFlatVector({1, std::nullopt, 2}), + makeFlatVector({false, false, false}), + makeFlatVector({1, 2, 3}), + makeNullableFlatVector({2, 2, std::nullopt})}); + + WindowTestBase::testWindowFunction( + {input}, + "first_value(c0 IGNORE NULLS)", + "partition by c1 order by c2", + "rows between 1 following and unbounded following", + expected); + WindowTestBase::testWindowFunction( + {input}, + "last_value(c0 IGNORE NULLS)", + "partition by c1 order by c2", + "rows between 1 following and unbounded following", + expected); +} + // These tests are added since DuckDB has issues with // CURRENT ROW frames. These tests will be replaced by DuckDB based // tests after it is upgraded to v0.8. From 320f578ce7a7128b75cd92502488a92d11fe4995 Mon Sep 17 00:00:00 2001 From: "Schierbeck, Cody" Date: Mon, 12 Feb 2024 10:53:38 -0800 Subject: [PATCH 04/38] Created an expression runner for Spark functions (#8341) Summary: velox_expression_runner_test is only linked to velox_functions_prestosql. Added a spark_expression_runner_test that is linked to velox_functions_spark to allow for running SparkSQL functions. See comments for examples. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8341 Reviewed By: Yuhta Differential Revision: D53357054 Pulled By: kgpai fbshipit-source-id: f9cf562775839d1e5b95397905896faba8f940da --- velox/docs/develop/testing/fuzzer.rst | 2 ++ velox/expression/tests/CMakeLists.txt | 5 +-- .../expression/tests/ExpressionRunnerTest.cpp | 34 ++++++++++++++++--- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/velox/docs/develop/testing/fuzzer.rst b/velox/docs/develop/testing/fuzzer.rst index cb883f69e4b1..57ee0e968310 100644 --- a/velox/docs/develop/testing/fuzzer.rst +++ b/velox/docs/develop/testing/fuzzer.rst @@ -235,6 +235,8 @@ ExpressionRunner supports the following flags: * ``--sql_path`` path to expression SQL that was created by the Fuzzer +* ``--registry`` function registry to use for evaluating expression. One of "presto" (default) or "spark". + * ``--complex_constant_path`` optional path to complex constants that aren't accurately expressable in SQL (Array, Map, Structs, ...). This is used with SQL file to reproduce the exact expression, not needed when the expression doesn't contain complex constants. * ``--lazy_column_list_path`` optional path for the file stored on-disk which contains a vector of column indices that specify which columns of the input row vector should be wrapped in lazy. This is used when the failing test included input columns that were lazy vector. diff --git a/velox/expression/tests/CMakeLists.txt b/velox/expression/tests/CMakeLists.txt index fb5fab232c86..ded7d6aa82c5 100644 --- a/velox/expression/tests/CMakeLists.txt +++ b/velox/expression/tests/CMakeLists.txt @@ -112,8 +112,9 @@ target_link_libraries( gtest_main) add_library(velox_expression_runner ExpressionRunner.cpp) -target_link_libraries(velox_expression_runner velox_expression_verifier - velox_functions_prestosql velox_parse_parser gtest) +target_link_libraries( + velox_expression_runner velox_expression_verifier velox_functions_prestosql + velox_functions_spark velox_parse_parser gtest) add_executable(velox_expression_runner_test ExpressionRunnerTest.cpp) target_link_libraries( diff --git a/velox/expression/tests/ExpressionRunnerTest.cpp b/velox/expression/tests/ExpressionRunnerTest.cpp index 08e976af4a57..c6e06d79141d 100644 --- a/velox/expression/tests/ExpressionRunnerTest.cpp +++ b/velox/expression/tests/ExpressionRunnerTest.cpp @@ -22,6 +22,7 @@ #include "velox/expression/tests/ExpressionVerifier.h" #include "velox/functions/prestosql/aggregates/RegisterAggregateFunctions.h" #include "velox/functions/prestosql/registration/RegistrationFunctions.h" +#include "velox/functions/sparksql/Register.h" #include "velox/vector/VectorSaver.h" using namespace facebook::velox; @@ -52,6 +53,12 @@ DEFINE_string( "flag are mutually exclusive. If both are specified, --sql is used and " "--sql_path is ignored."); +DEFINE_string( + registry, + "presto", + "Funciton registry to use for expression evaluation. Currently supported values are " + "presto and spark. Default is presto."); + DEFINE_string( result_path, "", @@ -83,16 +90,35 @@ static bool validateMode(const char* flagName, const std::string& value) { static const std::unordered_set kModes = { "common", "simplified", "verify", "query"}; if (kModes.count(value) != 1) { - std::cout << "Invalid value for --" << flagName << ": " << value << ". "; - std::cout << "Valid values are: " << folly::join(", ", kModes) << "." + std::cerr << "Invalid value for --" << flagName << ": " << value << ". "; + std::cerr << "Valid values are: " << folly::join(", ", kModes) << "." + << std::endl; + return false; + } + + return true; +} + +static bool validateRegistry(const char* flagName, const std::string& value) { + static const std::unordered_set kRegistries = { + "presto", "spark"}; + if (kRegistries.count(value) != 1) { + std::cerr << "Invalid value for --" << flagName << ": " << value << ". "; + std::cerr << "Valid values are: " << folly::join(", ", kRegistries) << "." << std::endl; return false; } + if (value == "spark") { + functions::sparksql::registerFunctions(""); + } else if (value == "presto") { + functions::prestosql::registerAllScalarFunctions(); + } return true; } DEFINE_validator(mode, &validateMode); +DEFINE_validator(registry, &validateRegistry); DEFINE_int32( num_rows, @@ -172,7 +198,7 @@ int main(int argc, char** argv) { } if (FLAGS_sql.empty() && FLAGS_sql_path.empty()) { - std::cout << "One of --sql or --sql_path flags must be set." << std::endl; + std::cerr << "One of --sql or --sql_path flags must be set." << std::endl; exit(1); } @@ -182,8 +208,6 @@ int main(int argc, char** argv) { VELOX_CHECK(!sql.empty()); } - functions::prestosql::registerAllScalarFunctions(); - aggregate::prestosql::registerAllAggregateFunctions(); test::ExpressionRunner::run( FLAGS_input_path, sql, From 080b7857139ca647e90a5cee81344f920a1be84d Mon Sep 17 00:00:00 2001 From: Wei He Date: Mon, 12 Feb 2024 10:58:33 -0800 Subject: [PATCH 05/38] Fix min_by/max_by(x, y, n) (#8566) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8566 Same as bug in min/max(x, n) fixed in https://github.com/facebookincubator/velox/pull/8311, min_by/max_by(x, y, n) also breaks the assumption of incremental window aggregation because their extractValues() methods has a side effect of clearing the accumulator. This diff fixes this issue by making the extractValues() methods of min_by/max_by(x, y, n) not clear the accumulators. Since Presto's min_by/max_by have the same bug (https://github.com/prestodb/presto/issues/21653). This fix will make Velox's min_by/max_by behave differently from Presto when used in Window operation, until https://github.com/prestodb/presto/issues/21653 is fixed. This diff fixes https://github.com/facebookincubator/velox/issues/8138. Reviewed By: bikramSingh91 Differential Revision: D53139892 fbshipit-source-id: 1323f22196e22554c0d880d20584a4ee4059b64c --- velox/docs/develop/aggregate-functions.rst | 10 +- .../aggregates/MinMaxByAggregates.cpp | 292 ++++++++++-------- .../tests/MinMaxByAggregationTest.cpp | 92 +++++- 3 files changed, 256 insertions(+), 138 deletions(-) diff --git a/velox/docs/develop/aggregate-functions.rst b/velox/docs/develop/aggregate-functions.rst index a67f0a4e7e8a..572c7446e16a 100644 --- a/velox/docs/develop/aggregate-functions.rst +++ b/velox/docs/develop/aggregate-functions.rst @@ -283,6 +283,9 @@ initialize all accumulators. The author can also optionally define a `destroy` function that is called when *this* accumulator object is destructed. +Notice that `writeIntermediateResult` and `writeFinalResult` are expected to not +modify contents in the accumulator. + addInput """""""" @@ -365,6 +368,9 @@ behavior. On the other hand, the C++ function signatures of `addInput`, `combine`, `writeIntermediateResult`, and `writeFinalResult` are different. +Same as the case for default-null behavior, `writeIntermediateResult` and +`writeFinalResult` are expected to not modify contents in the accumulator. + addInput """""""" @@ -605,6 +611,7 @@ After implementing the addRawInput() method, we proceed to adding logic for extr .. code-block:: c++ // Extracts partial results (used for partial and intermediate aggregations). + // This method is expected to not modify contents in accumulators. // @param groups Pointers to the start of the group rows. // @param numGroups Number of groups to extract results from. // @param result The result vector to store the results in. @@ -625,7 +632,8 @@ Next, we implement the extractValues() method that extracts final results from t .. code-block:: c++ - // Extracts final results (used for final and single aggregations). + // Extracts final results (used for final and single aggregations). This method + // is expected to not modify contents in accumulators. // @param groups Pointers to the start of the group rows. // @param numGroups Number of groups to extract results from. // @param result The result vector to store the results in. diff --git a/velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp b/velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp index 877134ceff88..0c81d61204b8 100644 --- a/velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp +++ b/velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include "velox/functions/lib/aggregates/MinMaxByAggregatesBase.h" #include "velox/functions/lib/aggregates/ValueSet.h" #include "velox/functions/prestosql/aggregates/AggregateNames.h" @@ -89,19 +90,18 @@ struct MinMaxByNAccumulator { int64_t n{0}; using Pair = std::pair>; - using Queue = - std::priority_queue>, Compare>; - Queue topPairs; + using Heap = std::vector>; + Heap heapValues; explicit MinMaxByNAccumulator(HashStringAllocator* allocator) - : topPairs{Compare{}, StlAllocator(allocator)} {} + : heapValues{StlAllocator(allocator)} {} int64_t getN() const { return n; } size_t size() const { - return topPairs.size(); + return heapValues.size(); } void checkAndSetN(DecodedVector& decodedN, vector_size_t row) { @@ -126,61 +126,64 @@ struct MinMaxByNAccumulator { void compareAndAdd(C comparison, std::optional value, Compare& comparator) { - if (topPairs.size() < n) { - topPairs.push({comparison, value}); + if (heapValues.size() < n) { + heapValues.push_back({comparison, value}); + std::push_heap(heapValues.begin(), heapValues.end(), comparator); } else { - const auto& topPair = topPairs.top(); + const auto& topPair = heapValues.front(); if (comparator.compare(comparison, topPair)) { - topPairs.pop(); - topPairs.push({comparison, value}); + std::pop_heap(heapValues.begin(), heapValues.end(), comparator); + heapValues.back() = std::make_pair(comparison, value); + std::push_heap(heapValues.begin(), heapValues.end(), comparator); } } } - /// Moves all values from 'topPairs' into 'rawValues' and 'rawValueNulls' - /// buffers. The queue of 'topPairs' will be empty after this call. + /// Extract all values from 'heapValues' into 'rawValues' and 'rawValueNulls' + /// buffers. The heap remains unchanged after the call. void extractValues( TRawValue* rawValues, uint64_t* rawValueNulls, - vector_size_t offset) { - const vector_size_t size = topPairs.size(); - for (auto i = size - 1; i >= 0; --i) { - const auto& topPair = topPairs.top(); + vector_size_t offset, + Compare& comparator) { + std::sort_heap(heapValues.begin(), heapValues.end(), comparator); + // Add heap elements to rawValues in ascending order. + for (int64_t i = 0; i < heapValues.size(); ++i) { + const auto& pair = heapValues[i]; const auto index = offset + i; - - const bool valueIsNull = !topPair.second.has_value(); + const bool valueIsNull = !pair.second.has_value(); bits::setNull(rawValueNulls, index, valueIsNull); if (!valueIsNull) { - RawValueExtractor::extract(rawValues, index, topPair.second.value()); + RawValueExtractor::extract(rawValues, index, pair.second.value()); } - - topPairs.pop(); } + std::make_heap(heapValues.begin(), heapValues.end(), comparator); } - /// Moves all pairs of (comparison, value) from 'topPairs' into - /// 'rawComparisons', 'rawValues' and 'rawValueNulls' buffers. The queue of - /// 'topPairs' will be empty after this call. + /// Moves all pairs of (comparison, value) from 'heapValues' into + /// 'rawComparisons', 'rawValues' and 'rawValueNulls' buffers. The heap + /// remains unchanged after the call. void extractPairs( TRawComparison* rawComparisons, TRawValue* rawValues, uint64_t* rawValueNulls, - vector_size_t offset) { - const vector_size_t size = topPairs.size(); - for (auto i = size - 1; i >= 0; --i) { - const auto& topPair = topPairs.top(); + vector_size_t offset, + Compare& comparator) { + std::sort_heap(heapValues.begin(), heapValues.end(), comparator); + // Add heap elements to rawComparisons and rawValues in ascending order. + for (int64_t i = 0; i < heapValues.size(); ++i) { + const auto& pair = heapValues[i]; const auto index = offset + i; - RawValueExtractor::extract(rawComparisons, index, topPair.first); + RawValueExtractor::extract(rawComparisons, index, pair.first); - const bool valueIsNull = !topPair.second.has_value(); + const bool valueIsNull = !pair.second.has_value(); bits::setNull(rawValueNulls, index, valueIsNull); if (!valueIsNull) { - RawValueExtractor::extract(rawValues, index, topPair.second.value()); + RawValueExtractor::extract(rawValues, index, pair.second.value()); } - - topPairs.pop(); } + std::make_heap(heapValues.begin(), heapValues.end(), comparator); } }; @@ -199,15 +202,18 @@ struct Extractor { void extractValues( MinMaxByNAccumulator* accumulator, - vector_size_t offset) { - accumulator->extractValues(rawValues, rawValueNulls, offset); + vector_size_t offset, + Compare& comparator) { + accumulator->extractValues(rawValues, rawValueNulls, offset, comparator); } void extractPairs( MinMaxByNAccumulator* accumulator, TRawComparison* rawComparisons, - vector_size_t offset) { - accumulator->extractPairs(rawComparisons, rawValues, rawValueNulls, offset); + vector_size_t offset, + Compare& comparator) { + accumulator->extractPairs( + rawComparisons, rawValues, rawValueNulls, offset, comparator); } }; @@ -221,10 +227,8 @@ struct MinMaxByNStringViewAccumulator { : base{allocator}, valueSet{allocator} {} ~MinMaxByNStringViewAccumulator() { - while (!base.topPairs.empty()) { - auto& pair = base.topPairs.top(); - freePair(pair); - base.topPairs.pop(); + for (auto i = 0; i < base.heapValues.size(); ++i) { + freePair(base.heapValues[i]); } } @@ -242,47 +246,52 @@ struct MinMaxByNStringViewAccumulator { void compareAndAdd(C comparison, std::optional value, Compare& comparator) { - if (base.topPairs.size() < base.n) { - addToAccumulator(comparison, value); + if (base.heapValues.size() < base.n) { + addToAccumulator(comparison, value, comparator); } else { - const auto& topPair = base.topPairs.top(); + const auto& topPair = base.heapValues.front(); if (comparator.compare(comparison, topPair)) { - freePair(topPair); - base.topPairs.pop(); - addToAccumulator(comparison, value); + std::pop_heap( + base.heapValues.begin(), base.heapValues.end(), comparator); + base.heapValues.pop_back(); + addToAccumulator(comparison, value, comparator); } } } - /// Moves all values from 'topPairs' into 'values' - /// buffers. The queue of 'topPairs' will be empty after this call. - void extractValues(FlatVector& values, vector_size_t offset) { - const vector_size_t size = base.topPairs.size(); - for (auto i = size - 1; i >= 0; --i) { - const auto& pair = base.topPairs.top(); + /// Extract all values from 'heapValues' into 'rawValues' and 'rawValueNulls' + /// buffers. The heap remains unchanged after the call. + void extractValues( + FlatVector& values, + vector_size_t offset, + Compare& comparator) { + std::sort_heap(base.heapValues.begin(), base.heapValues.end(), comparator); + // Add heap elements to values in ascending order. + for (int64_t i = 0; i < base.heapValues.size(); ++i) { + const auto& pair = base.heapValues[i]; extractValue(pair, values, offset + i); - freePair(pair); - base.topPairs.pop(); } + std::make_heap(base.heapValues.begin(), base.heapValues.end(), comparator); } - /// Moves all pairs of (comparison, value) from 'topPairs' into - /// 'rawComparisons' buffer and 'values' vector. The queue of - /// 'topPairs' will be empty after this call. + /// Moves all pairs of (comparison, value) from 'heapValues' into + /// 'rawComparisons', 'rawValues' and 'rawValueNulls' buffers. The heap + /// remains unchanged after the call. void extractPairs( FlatVector& compares, FlatVector& values, - vector_size_t offset) { - const vector_size_t size = base.topPairs.size(); - for (auto i = size - 1; i >= 0; --i) { - const auto& topPair = base.topPairs.top(); + vector_size_t offset, + Compare& comparator) { + std::sort_heap(base.heapValues.begin(), base.heapValues.end(), comparator); + // Add heap elements to compares and values in ascending order. + for (int64_t i = 0; i < base.heapValues.size(); ++i) { + const auto& pair = base.heapValues[i]; const auto index = offset + i; - extractCompare(topPair, compares, index); - extractValue(topPair, values, index); - freePair(topPair); - base.topPairs.pop(); + extractCompare(pair, compares, index); + extractValue(pair, values, index); } + std::make_heap(base.heapValues.begin(), base.heapValues.end(), comparator); } private: @@ -296,48 +305,50 @@ struct MinMaxByNStringViewAccumulator { return valueSet.write(*value); } - void addToAccumulator(C comparison, std::optional value) { + void + addToAccumulator(C comparison, std::optional value, Compare& comparator) { if constexpr ( std::is_same_v && std::is_same_v) { - base.topPairs.push({valueSet.write(comparison), writeString(value)}); + base.heapValues.push_back( + std::make_pair(valueSet.write(comparison), writeString(value))); } else if constexpr (std::is_same_v) { - base.topPairs.push({comparison, writeString(value)}); + base.heapValues.push_back(std::make_pair(comparison, writeString(value))); } else { static_assert( std::is_same_v, "At least one of V and C must be StringView."); - base.topPairs.push({valueSet.write(comparison), value}); + base.heapValues.push_back( + std::make_pair(valueSet.write(comparison), value)); } + std::push_heap(base.heapValues.begin(), base.heapValues.end(), comparator); } - void freePair(typename BaseType::Queue::const_reference topPair) { + void freePair(typename BaseType::Heap::const_reference pair) { if constexpr (std::is_same_v) { - valueSet.free(topPair.first); + valueSet.free(pair.first); } if constexpr (std::is_same_v) { - if (topPair.second.has_value()) { - valueSet.free(*topPair.second); + if (pair.second.has_value()) { + valueSet.free(*pair.second); } } } - void extractValue( - const Pair& topPair, - FlatVector& values, - vector_size_t index) { - const bool valueIsNull = !topPair.second.has_value(); + void + extractValue(const Pair& pair, FlatVector& values, vector_size_t index) { + const bool valueIsNull = !pair.second.has_value(); values.setNull(index, valueIsNull); if (!valueIsNull) { - values.set(index, topPair.second.value()); + values.set(index, pair.second.value()); } } void extractCompare( - const Pair& topPair, + const Pair& pair, FlatVector& compares, vector_size_t index) { compares.setNull(index, false); - compares.set(index, topPair.first); + compares.set(index, pair.first); } }; @@ -353,15 +364,17 @@ struct StringViewExtractor { void extractValues( MinMaxByNStringViewAccumulator* accumulator, - vector_size_t offset) { - accumulator->extractValues(values, offset); + vector_size_t offset, + Compare& comparator) { + accumulator->extractValues(values, offset, comparator); } void extractPairs( MinMaxByNStringViewAccumulator* accumulator, - vector_size_t offset) { + vector_size_t offset, + Compare& comparator) { VELOX_DCHECK_NOT_NULL(compares); - accumulator->extractPairs(*compares, values, offset); + accumulator->extractPairs(*compares, values, offset, comparator); } }; @@ -379,10 +392,8 @@ struct MinMaxByNComplexTypeAccumulator { : base{allocator}, valueSet{allocator} {} ~MinMaxByNComplexTypeAccumulator() { - while (!base.topPairs.empty()) { - auto& pair = base.topPairs.top(); - freePair(pair); - base.topPairs.pop(); + for (auto i = 0; i < base.heapValues.size(); ++i) { + freePair(base.heapValues[i]); } } @@ -403,62 +414,69 @@ struct MinMaxByNComplexTypeAccumulator { DecodedVector& decoded, vector_size_t index, Compare& comparator) { - if (base.topPairs.size() < base.n) { + if (base.heapValues.size() < base.n) { auto position = writeComplex(decoded, index); - addToAccumulator(comparison, position); + addToAccumulator(comparison, position, comparator); } else { - const auto& topPair = base.topPairs.top(); + const auto& topPair = base.heapValues.front(); if (comparator.compare(comparison, topPair)) { - freePair(topPair); - base.topPairs.pop(); - + std::pop_heap( + base.heapValues.begin(), base.heapValues.end(), comparator); auto position = writeComplex(decoded, index); - addToAccumulator(comparison, position); + base.heapValues.pop_back(); + addToAccumulator(comparison, position, comparator); } } } - /// Moves all values from 'topPairs' into 'values' vector. The queue of - /// 'topPairs' will be empty after this call. - void extractValues(BaseVector& values, vector_size_t offset) { - const vector_size_t size = base.topPairs.size(); - for (auto i = size - 1; i >= 0; --i) { - const auto& pair = base.topPairs.top(); + /// Extract all values from 'heapValues' into 'rawValues' and 'rawValueNulls' + /// buffers. The heap remains unchanged after the call. + void + extractValues(BaseVector& values, vector_size_t offset, Compare& comparator) { + std::sort_heap(base.heapValues.begin(), base.heapValues.end(), comparator); + // Add heap elements to values in ascending order. + for (int64_t i = 0; i < base.heapValues.size(); ++i) { + const auto& pair = base.heapValues[i]; extractValue(pair, values, offset + i); - freePair(pair); - base.topPairs.pop(); } + std::make_heap(base.heapValues.begin(), base.heapValues.end(), comparator); } - /// Moves all pairs of (comparison, value) from 'topPairs' into - /// 'rawComparisons' buffer and 'values' vector. The queue of - /// 'topPairs' will be empty after this call. + /// Moves all pairs of (comparison, value) from 'heapValues' into + /// 'rawComparisons', 'rawValues' and 'rawValueNulls' buffers. The heap + /// remains unchanged after the call. void extractPairs( FlatVector& compares, BaseVector& values, - vector_size_t offset) { - const vector_size_t size = base.topPairs.size(); - for (auto i = size - 1; i >= 0; --i) { - const auto& topPair = base.topPairs.top(); + vector_size_t offset, + Compare& comparator) { + std::sort_heap(base.heapValues.begin(), base.heapValues.end(), comparator); + // Add heap elements to compares and values in ascending order. + for (int64_t i = 0; i < base.heapValues.size(); ++i) { + const auto& pair = base.heapValues[i]; const auto index = offset + i; - extractCompare(topPair, compares, index); - extractValue(topPair, values, index); - freePair(topPair); - base.topPairs.pop(); + extractCompare(pair, compares, index); + extractValue(pair, values, index); } + std::make_heap(base.heapValues.begin(), base.heapValues.end(), comparator); } private: using V = HashStringAllocator::Position; using Pair = typename MinMaxByNAccumulator::Pair; - void addToAccumulator(C comparison, const std::optional& position) { + void addToAccumulator( + C comparison, + const std::optional& position, + Compare& comparator) { if constexpr (std::is_same_v) { - base.topPairs.push({valueSet.write(comparison), position}); + base.heapValues.push_back( + std::make_pair(valueSet.write(comparison), position)); } else { - base.topPairs.push({comparison, position}); + base.heapValues.push_back(std::make_pair(comparison, position)); } + std::push_heap(base.heapValues.begin(), base.heapValues.end(), comparator); } std::optional writeComplex( @@ -472,31 +490,30 @@ struct MinMaxByNComplexTypeAccumulator { return position; } - void freePair(typename BaseType::Queue::const_reference topPair) { + void freePair(typename BaseType::Heap::const_reference pair) { if constexpr (std::is_same_v) { - valueSet.free(topPair.first); + valueSet.free(pair.first); } - if (topPair.second.has_value()) { - valueSet.free(topPair.second->header); + if (pair.second.has_value()) { + valueSet.free(pair.second->header); } } - void - extractValue(const Pair& topPair, BaseVector& values, vector_size_t index) { - const bool valueIsNull = !topPair.second.has_value(); + void extractValue(const Pair& pair, BaseVector& values, vector_size_t index) { + const bool valueIsNull = !pair.second.has_value(); values.setNull(index, valueIsNull); if (!valueIsNull) { - auto position = topPair.second.value(); + auto position = pair.second.value(); valueSet.read(&values, index, position.header); } } void extractCompare( - const Pair& topPair, + const Pair& pair, FlatVector& compares, vector_size_t index) { compares.setNull(index, false); - compares.set(index, topPair.first); + compares.set(index, pair.first); } }; // namespace @@ -511,15 +528,17 @@ struct ComplexTypeExtractor { void extractValues( MinMaxByNComplexTypeAccumulator* accumulator, - vector_size_t offset) { - accumulator->extractValues(values, offset); + vector_size_t offset, + Compare& comparator) { + accumulator->extractValues(values, offset, comparator); } void extractPairs( MinMaxByNComplexTypeAccumulator* accumulator, - vector_size_t offset) { + vector_size_t offset, + Compare& comparator) { VELOX_DCHECK_NOT_NULL(compares); - accumulator->extractPairs(*compares, values, offset); + accumulator->extractPairs(*compares, values, offset, comparator); } }; @@ -658,7 +677,7 @@ class MinMaxByNAggregate : public exec::Aggregate { rawOffsets[i] = offset; rawSizes[i] = size; - extractor->extractValues(accumulator, offset); + extractor->extractValues(accumulator, offset, comparator_); offset += size; } @@ -720,9 +739,10 @@ class MinMaxByNAggregate : public exec::Aggregate { if constexpr ( std::is_same_v || std::is_same_v || std::is_same_v) { - extractor->extractPairs(accumulator, offset); + extractor->extractPairs(accumulator, offset, comparator_); } else { - extractor->extractPairs(accumulator, rawComparisons, offset); + extractor->extractPairs( + accumulator, rawComparisons, offset, comparator_); } offset += size; diff --git a/velox/functions/prestosql/aggregates/tests/MinMaxByAggregationTest.cpp b/velox/functions/prestosql/aggregates/tests/MinMaxByAggregationTest.cpp index 5ac1386527e6..ecc17ad1d7ec 100644 --- a/velox/functions/prestosql/aggregates/tests/MinMaxByAggregationTest.cpp +++ b/velox/functions/prestosql/aggregates/tests/MinMaxByAggregationTest.cpp @@ -1370,7 +1370,6 @@ class MinMaxByNTest : public AggregationTestBase { AggregationTestBase::SetUp(); AggregationTestBase::allowInputShuffle(); AggregationTestBase::enableTestStreaming(); - AggregationTestBase::disableTestIncremental(); } }; @@ -1764,6 +1763,10 @@ TEST_F(MinMaxByNTest, sortedGroupBy) { } TEST_F(MinMaxByNTest, variableN) { + // Tests below check the error behavior on invalid inputs, so testIncremental + // is not needed for these cases. + AggregationTestBase::disableTestIncremental(); + auto data = makeRowVector({ makeFlatVector({1, 2, 3, 4, 5, 6, 7}), makeFlatVector({77, 66, 55, 44, 33, 22, 11}), @@ -1819,6 +1822,8 @@ TEST_F(MinMaxByNTest, variableN) { VELOX_ASSERT_THROW( AssertQueryBuilder(plan).copyResults(pool()), "third argument of max_by/min_by must be a constant for all rows in a group"); + + AggregationTestBase::enableTestIncremental(); } TEST_F(MinMaxByNTest, globalRow) { @@ -2111,5 +2116,90 @@ TEST_F(MinMaxByNTest, stringComparison) { } } +TEST_F(MinMaxByNTest, incrementalWindow) { + // Test that min_by(x, x, 10) and max_by(x, x, 10) produce correct results + // when used in window operation with incremental frames. + std::vector inputs = { + makeFlatVector({1, 2}), + makeFlatVector({"1"_sv, "2"_sv}), + makeArrayVector({{"1"_sv}, {"2"_sv}}), + makeFlatVector({Timestamp(0, 0), Timestamp(0, 1)}), + makeFlatVector({10, 10}), + makeFlatVector({false, false}), + makeFlatVector({0, 1})}; + auto data = makeRowVector(inputs); + auto result = inputs; + + // Test primitive type. + { + auto plan = + PlanBuilder() + .values({data}) + .window( + {"max_by(c0, c0, c4) over (partition by c5 order by c6 asc)"}) + .planNode(); + + result.push_back(makeArrayVector({{1}, {2, 1}})); + AssertQueryBuilder(plan).assertResults(makeRowVector(result)); + + plan = + PlanBuilder() + .values({data}) + .window( + {"min_by(c0, c0, c4) over (partition by c5 order by c6 asc)"}) + .planNode(); + result.back() = makeArrayVector({{1}, {1, 2}}); + AssertQueryBuilder(plan).assertResults(makeRowVector(result)); + } + + // Test varchar type. + { + auto plan = + PlanBuilder() + .values({data}) + .window( + {"max_by(c1, c1, c4) over (partition by c5 order by c6 asc)"}) + .planNode(); + + result.back() = makeArrayVector({{"1"_sv}, {"2"_sv, "1"_sv}}); + AssertQueryBuilder(plan).assertResults(makeRowVector(result)); + + plan = + PlanBuilder() + .values({data}) + .window( + {"min_by(c1, c1, c4) over (partition by c5 order by c6 asc)"}) + .planNode(); + + result.back() = makeArrayVector({{"1"_sv}, {"1"_sv, "2"_sv}}); + AssertQueryBuilder(plan).assertResults(makeRowVector(result)); + } + + // Test complex type. + { + auto plan = + PlanBuilder() + .values({data}) + .window( + {"max_by(c2, c3, c4) over (partition by c5 order by c6 asc)"}) + .planNode(); + + result.back() = makeNullableNestedArrayVector( + {{{{{"1"_sv}}}}, {{{{"2"_sv}}, {{"1"_sv}}}}}); + AssertQueryBuilder(plan).assertResults(makeRowVector(result)); + + plan = + PlanBuilder() + .values({data}) + .window( + {"min_by(c2, c3, c4) over (partition by c5 order by c6 asc)"}) + .planNode(); + + result.back() = makeNullableNestedArrayVector( + {{{{{"1"_sv}}}}, {{{{"1"_sv}}, {{"2"_sv}}}}}); + AssertQueryBuilder(plan).assertResults(makeRowVector(result)); + } +} + } // namespace } // namespace facebook::velox::aggregate::test From a3a57cbf5a80ef9c5b31460f9f0fb35bb110afbe Mon Sep 17 00:00:00 2001 From: Ankita Victor Date: Mon, 12 Feb 2024 11:51:36 -0800 Subject: [PATCH 06/38] Add date_from_unix_date Spark function (#8672) Summary: Spark docs - https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.functions.date_from_unix_date.html Fixes https://github.com/facebookincubator/velox/issues/8671 Pull Request resolved: https://github.com/facebookincubator/velox/pull/8672 Reviewed By: bikramSingh91 Differential Revision: D53569359 Pulled By: mbasmanova fbshipit-source-id: 71677ee9e222641d04cb4f798949e643ee40376e --- velox/docs/functions/spark/datetime.rst | 7 ++++++ velox/functions/sparksql/DateTimeFunctions.h | 9 ++++++++ velox/functions/sparksql/Register.cpp | 3 +++ .../sparksql/tests/DateTimeFunctionsTest.cpp | 22 +++++++++++++++++++ 4 files changed, 41 insertions(+) diff --git a/velox/docs/functions/spark/datetime.rst b/velox/docs/functions/spark/datetime.rst index 138d3ce68a95..3ac392536524 100644 --- a/velox/docs/functions/spark/datetime.rst +++ b/velox/docs/functions/spark/datetime.rst @@ -34,6 +34,13 @@ These functions support TIMESTAMP and DATE input types. deducted from ``start_date``. Supported types for ``num_days`` are: TINYINT, SMALLINT, INTEGER. +.. spark:function:: date_from_unix_date(integer) -> date + + Creates date from the number of days since 1970-01-01 in either direction. Returns null when input is null. + + SELECT date_from_unix_date(1); -- '1970-01-02' + SELECT date_from_unix_date(-1); -- '1969-12-31' + .. spark:function:: date_sub(start_date, num_days) -> date Returns the date that is ``num_days`` before ``start_date``. According to the inputs, diff --git a/velox/functions/sparksql/DateTimeFunctions.h b/velox/functions/sparksql/DateTimeFunctions.h index 7d6c184eb1bb..94a7f21317e5 100644 --- a/velox/functions/sparksql/DateTimeFunctions.h +++ b/velox/functions/sparksql/DateTimeFunctions.h @@ -360,6 +360,15 @@ struct LastDayFunction { } }; +template +struct DateFromUnixDateFunction { + VELOX_DEFINE_FUNCTION_TYPES(T); + + FOLLY_ALWAYS_INLINE void call(out_type& result, const int32_t& value) { + result = value; + } +}; + template struct DateAddFunction { VELOX_DEFINE_FUNCTION_TYPES(T); diff --git a/velox/functions/sparksql/Register.cpp b/velox/functions/sparksql/Register.cpp index 7b028f6af978..a24c0bf215ae 100644 --- a/velox/functions/sparksql/Register.cpp +++ b/velox/functions/sparksql/Register.cpp @@ -292,6 +292,9 @@ void registerFunctions(const std::string& prefix) { registerFunction({prefix + "date_add"}); registerFunction({prefix + "date_add"}); + registerFunction( + {prefix + "date_from_unix_date"}); + registerFunction({prefix + "date_sub"}); registerFunction({prefix + "date_sub"}); registerFunction({prefix + "date_sub"}); diff --git a/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp b/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp index 66562e3c735b..b7cfbe4d694e 100644 --- a/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp +++ b/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp @@ -245,6 +245,28 @@ TEST_F(DateTimeFunctionsTest, lastDay) { EXPECT_EQ(lastDayFunc(std::nullopt), std::nullopt); } +TEST_F(DateTimeFunctionsTest, dateFromUnixDate) { + const auto dateFromUnixDate = [&](std::optional value) { + return evaluateOnce("date_from_unix_date(c0)", value); + }; + + // Basic tests + EXPECT_EQ(parseDate("1970-01-01"), dateFromUnixDate(0)); + EXPECT_EQ(parseDate("1970-01-02"), dateFromUnixDate(1)); + EXPECT_EQ(parseDate("1969-12-31"), dateFromUnixDate(-1)); + EXPECT_EQ(parseDate("1970-02-01"), dateFromUnixDate(31)); + EXPECT_EQ(parseDate("1971-01-31"), dateFromUnixDate(395)); + EXPECT_EQ(parseDate("1971-01-01"), dateFromUnixDate(365)); + + // Leap year tests + EXPECT_EQ(parseDate("1972-02-29"), dateFromUnixDate(365 + 365 + 30 + 29)); + EXPECT_EQ(parseDate("1971-03-01"), dateFromUnixDate(365 + 30 + 28 + 1)); + + // Min and max value tests + EXPECT_EQ(parseDate("5881580-07-11"), dateFromUnixDate(kMax)); + EXPECT_EQ(parseDate("-5877641-06-23"), dateFromUnixDate(kMin)); +} + TEST_F(DateTimeFunctionsTest, dateAdd) { const auto dateAdd = [&](const std::string& dateStr, std::optional value) { From 7227ff82291670874cb151d13d60bb3a1a748617 Mon Sep 17 00:00:00 2001 From: Wei He Date: Mon, 12 Feb 2024 13:28:11 -0800 Subject: [PATCH 07/38] Fix arbitrary() to always return the first non-null value in Window operation (#8640) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8640 The implementation of arbitrary() intends to always return the first non-null value, but it doesn't for complex-typed inputs when used in Window operation with incremental frames. This is because ArbitraryFunction::addSingleGroupRawInput() still updates the accumulator even if the accumulator already has a value. This diff fixes this issue by making ArbitraryFunction::addSingleGroupRawInput() return immediately if accumulator already has a value. This diff fixes https://github.com/facebookincubator/velox/issues/8593. Reviewed By: kgpai Differential Revision: D53328253 fbshipit-source-id: 803261dce0ec1fc52187947b1f9316dfd814c3fd --- .../aggregates/ArbitraryAggregate.cpp | 6 +++- .../aggregates/tests/ArbitraryTest.cpp | 35 +++++++++++++++++++ .../prestosql/aggregates/tests/CMakeLists.txt | 1 + 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/velox/functions/prestosql/aggregates/ArbitraryAggregate.cpp b/velox/functions/prestosql/aggregates/ArbitraryAggregate.cpp index fd7197430ba0..6bab95ba7f71 100644 --- a/velox/functions/prestosql/aggregates/ArbitraryAggregate.cpp +++ b/velox/functions/prestosql/aggregates/ArbitraryAggregate.cpp @@ -226,6 +226,11 @@ class NonNumericArbitrary : public exec::Aggregate { const SelectivityVector& rows, const std::vector& args, bool /*unused*/) override { + auto* accumulator = value(group); + if (accumulator->hasValue()) { + return; + } + DecodedVector decoded(*args[0], rows, true); if (decoded.isConstantMapping() && decoded.isNullAt(0)) { // nothing to do; all values are nulls @@ -234,7 +239,6 @@ class NonNumericArbitrary : public exec::Aggregate { const auto* indices = decoded.indices(); const auto* baseVector = decoded.base(); - auto* accumulator = value(group); // Find the first non-null value. rows.testSelected([&](vector_size_t i) { if (!decoded.isNullAt(i)) { diff --git a/velox/functions/prestosql/aggregates/tests/ArbitraryTest.cpp b/velox/functions/prestosql/aggregates/tests/ArbitraryTest.cpp index fcab97ce63db..93082cce97a5 100644 --- a/velox/functions/prestosql/aggregates/tests/ArbitraryTest.cpp +++ b/velox/functions/prestosql/aggregates/tests/ArbitraryTest.cpp @@ -16,9 +16,11 @@ #include "velox/exec/tests/utils/PlanBuilder.h" #include "velox/functions/lib/aggregates/tests/utils/AggregationTestBase.h" +#include "velox/functions/lib/window/tests/WindowTestBase.h" using namespace facebook::velox::exec::test; using namespace facebook::velox::functions::aggregate::test; +using namespace facebook::velox::window::test; namespace facebook::velox::aggregate::test { @@ -367,5 +369,38 @@ TEST_F(ArbitraryTest, interval) { testAggregations({data}, {}, {"arbitrary(c2)"}, "SELECT null"); } +class ArbitraryWindowTest : public WindowTestBase {}; + +TEST_F(ArbitraryWindowTest, basic) { + auto data = makeRowVector( + {makeFlatVector({1, 2, 3, 4, 5}), + makeArrayVector({{1.0}, {2.0}, {3.0}, {4.0}, {5.0}}), + makeFlatVector({false, false, false, false, false})}); + + auto expected = makeRowVector( + {makeFlatVector({1, 2, 3, 4, 5}), + makeArrayVector({{1.0}, {2.0}, {3.0}, {4.0}, {5.0}}), + makeFlatVector({false, false, false, false, false}), + makeFlatVector({1, 1, 1, 1, 1})}); + window::test::WindowTestBase::testWindowFunction( + {data}, + "arbitrary(c0)", + "partition by c2 order by c0", + "range between unbounded preceding and current row", + expected); + + expected = makeRowVector( + {makeFlatVector({1, 2, 3, 4, 5}), + makeArrayVector({{1.0}, {2.0}, {3.0}, {4.0}, {5.0}}), + makeFlatVector({false, false, false, false, false}), + makeArrayVector({{1.0}, {1.0}, {1.0}, {1.0}, {1.0}})}); + window::test::WindowTestBase::testWindowFunction( + {data}, + "arbitrary(c1)", + "partition by c2 order by c0", + "range between unbounded preceding and current row", + expected); +} + } // namespace } // namespace facebook::velox::aggregate::test diff --git a/velox/functions/prestosql/aggregates/tests/CMakeLists.txt b/velox/functions/prestosql/aggregates/tests/CMakeLists.txt index 51b0bebd8f37..0ded4a46fe9f 100644 --- a/velox/functions/prestosql/aggregates/tests/CMakeLists.txt +++ b/velox/functions/prestosql/aggregates/tests/CMakeLists.txt @@ -64,6 +64,7 @@ target_link_libraries( velox_file velox_functions_aggregates velox_functions_aggregates_test_lib + velox_functions_window_test_lib velox_functions_test_lib velox_functions_prestosql velox_functions_lib From 42b10d9432f1f211e9641d327282f1dfc4dc2325 Mon Sep 17 00:00:00 2001 From: Sergey Pershin Date: Mon, 12 Feb 2024 17:13:19 -0800 Subject: [PATCH 08/38] Fix crash in parseSerdeParameters() (#8730) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8730 We simply didn't check `nullStringIt` for being invalid before using it. Reviewed By: gggrace14 Differential Revision: D53676033 fbshipit-source-id: aea451e7995be84a06d86ae17eee39c26747e04b --- velox/connectors/hive/HiveConnectorUtil.cpp | 4 +- velox/connectors/hive/tests/CMakeLists.txt | 1 + .../hive/tests/HiveConnectorUtilTest.cpp | 205 ++++++++++++++++++ velox/dwio/common/Options.h | 7 + 4 files changed, 216 insertions(+), 1 deletion(-) create mode 100644 velox/connectors/hive/tests/HiveConnectorUtilTest.cpp diff --git a/velox/connectors/hive/HiveConnectorUtil.cpp b/velox/connectors/hive/HiveConnectorUtil.cpp index 464d7560691a..a0bc7fbd046e 100644 --- a/velox/connectors/hive/HiveConnectorUtil.cpp +++ b/velox/connectors/hive/HiveConnectorUtil.cpp @@ -418,7 +418,9 @@ std::unique_ptr parseSerdeParameters( } auto serDeOptions = std::make_unique( fieldDelim, collectionDelim, mapKeyDelim); - serDeOptions->nullString = nullStringIt->second; + if (nullStringIt != tableParameters.end()) { + serDeOptions->nullString = nullStringIt->second; + } return serDeOptions; } diff --git a/velox/connectors/hive/tests/CMakeLists.txt b/velox/connectors/hive/tests/CMakeLists.txt index c98db338002e..f84b2eb8ccfc 100644 --- a/velox/connectors/hive/tests/CMakeLists.txt +++ b/velox/connectors/hive/tests/CMakeLists.txt @@ -18,6 +18,7 @@ add_executable( FileHandleTest.cpp HivePartitionUtilTest.cpp HiveConnectorTest.cpp + HiveConnectorUtilTest.cpp HiveConnectorSerDeTest.cpp PartitionIdGeneratorTest.cpp TableHandleTest.cpp diff --git a/velox/connectors/hive/tests/HiveConnectorUtilTest.cpp b/velox/connectors/hive/tests/HiveConnectorUtilTest.cpp new file mode 100644 index 000000000000..5cd33e13d975 --- /dev/null +++ b/velox/connectors/hive/tests/HiveConnectorUtilTest.cpp @@ -0,0 +1,205 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include "velox/exec/tests/utils/HiveConnectorTestBase.h" + +#include "velox/connectors/hive/HiveConfig.h" +#include "velox/connectors/hive/HiveConnectorSplit.h" +#include "velox/connectors/hive/HiveConnectorUtil.h" +#include "velox/connectors/hive/TableHandle.h" +#include "velox/core/Config.h" + +namespace facebook::velox::connector { + +using namespace dwio::common; + +class HiveConnectorUtilTest : public exec::test::HiveConnectorTestBase { + protected: + static bool compareSerDeOptions( + const SerDeOptions& l, + const SerDeOptions& r) { + return l.isEscaped == r.isEscaped && l.escapeChar == r.escapeChar && + l.lastColumnTakesRest == r.lastColumnTakesRest && + l.nullString == r.nullString && l.separators == r.separators; + } + + std::shared_ptr pool_ = + memory::memoryManager()->addLeafPool(); +}; + +TEST_F(HiveConnectorUtilTest, configureReaderOptions) { + core::MemConfig sessionProperties; + auto hiveConfig = + std::make_shared(std::make_shared()); + const std::unordered_map> + partitionKeys; + const std::unordered_map customSplitInfo; + + // Dynamic parameters. + dwio::common::ReaderOptions readerOptions(pool_.get()); + FileFormat fileFormat{FileFormat::DWRF}; + std::unordered_map tableParameters; + std::unordered_map serdeParameters; + SerDeOptions expectedSerDe; + + auto createTableHandle = [&]() { + return std::make_shared( + "testConnectorId", + "testTable", + false, + hive::SubfieldFilters{}, + nullptr, + nullptr, + tableParameters); + }; + + auto createSplit = [&]() { + return std::make_shared( + "testConnectorId", + "/tmp/", + fileFormat, + 0UL, + std::numeric_limits::max(), + partitionKeys, + std::nullopt, + customSplitInfo, + nullptr, + serdeParameters); + }; + + auto performConfigure = [&]() { + auto tableHandle = createTableHandle(); + auto split = createSplit(); + configureReaderOptions( + readerOptions, hiveConfig, &sessionProperties, tableHandle, split); + }; + + auto clearDynamicParameters = [&](FileFormat newFileFormat) { + readerOptions = dwio::common::ReaderOptions(pool_.get()); + fileFormat = newFileFormat; + tableParameters.clear(); + serdeParameters.clear(); + expectedSerDe = SerDeOptions{}; + }; + + // Default. + performConfigure(); + EXPECT_EQ(readerOptions.getFileFormat(), fileFormat); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + EXPECT_EQ(readerOptions.maxCoalesceBytes(), hiveConfig->maxCoalescedBytes()); + EXPECT_EQ( + readerOptions.maxCoalesceDistance(), + hiveConfig->maxCoalescedDistanceBytes()); + EXPECT_EQ( + readerOptions.isFileColumnNamesReadAsLowerCase(), + hiveConfig->isFileColumnNamesReadAsLowerCase(&sessionProperties)); + EXPECT_EQ( + readerOptions.isUseColumnNamesForColumnMapping(), + hiveConfig->isOrcUseColumnNames(&sessionProperties)); + EXPECT_EQ( + readerOptions.getFooterEstimatedSize(), + hiveConfig->footerEstimatedSize()); + EXPECT_EQ( + readerOptions.getFilePreloadThreshold(), + hiveConfig->filePreloadThreshold()); + + // Modify field delimiter and change the file format. + clearDynamicParameters(FileFormat::TEXT); + serdeParameters[SerDeOptions::kFieldDelim] = '\t'; + expectedSerDe.separators[size_t(SerDeSeparator::FIELD_DELIM)] = '\t'; + performConfigure(); + EXPECT_EQ(readerOptions.getFileFormat(), fileFormat); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + + // Modify collection delimiter. + clearDynamicParameters(FileFormat::TEXT); + serdeParameters[SerDeOptions::kCollectionDelim] = '='; + expectedSerDe.separators[size_t(SerDeSeparator::COLLECTION_DELIM)] = '='; + performConfigure(); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + + // Modify map key delimiter. + clearDynamicParameters(FileFormat::TEXT); + serdeParameters[SerDeOptions::kMapKeyDelim] = '&'; + expectedSerDe.separators[size_t(SerDeSeparator::MAP_KEY_DELIM)] = '&'; + performConfigure(); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + + // Modify null string. + clearDynamicParameters(FileFormat::TEXT); + tableParameters[TableParameter::kSerializationNullFormat] = "x-x"; + expectedSerDe.nullString = "x-x"; + performConfigure(); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + + // Modify all previous together. + clearDynamicParameters(FileFormat::TEXT); + serdeParameters[SerDeOptions::kFieldDelim] = '~'; + expectedSerDe.separators[size_t(SerDeSeparator::FIELD_DELIM)] = '~'; + serdeParameters[SerDeOptions::kCollectionDelim] = '$'; + expectedSerDe.separators[size_t(SerDeSeparator::COLLECTION_DELIM)] = '$'; + serdeParameters[SerDeOptions::kMapKeyDelim] = '*'; + expectedSerDe.separators[size_t(SerDeSeparator::MAP_KEY_DELIM)] = '*'; + tableParameters[TableParameter::kSerializationNullFormat] = ""; + expectedSerDe.nullString = ""; + performConfigure(); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + EXPECT_TRUE( + compareSerDeOptions(readerOptions.getSerDeOptions(), expectedSerDe)); + + // Tests other custom reader options. + clearDynamicParameters(FileFormat::TEXT); + std::unordered_map customHiveConfigProps; + customHiveConfigProps[hive::HiveConfig::kMaxCoalescedBytes] = "129"; + customHiveConfigProps[hive::HiveConfig::kMaxCoalescedDistanceBytes] = "513"; + customHiveConfigProps[hive::HiveConfig::kFileColumnNamesReadAsLowerCase] = + "true"; + customHiveConfigProps[hive::HiveConfig::kOrcUseColumnNames] = "true"; + customHiveConfigProps[hive::HiveConfig::kFooterEstimatedSize] = "1111"; + customHiveConfigProps[hive::HiveConfig::kFilePreloadThreshold] = "9999"; + hiveConfig = std::make_shared( + std::make_shared(customHiveConfigProps)); + performConfigure(); + EXPECT_EQ(readerOptions.maxCoalesceBytes(), hiveConfig->maxCoalescedBytes()); + EXPECT_EQ( + readerOptions.maxCoalesceDistance(), + hiveConfig->maxCoalescedDistanceBytes()); + EXPECT_EQ( + readerOptions.isFileColumnNamesReadAsLowerCase(), + hiveConfig->isFileColumnNamesReadAsLowerCase(&sessionProperties)); + EXPECT_EQ( + readerOptions.isUseColumnNamesForColumnMapping(), + hiveConfig->isOrcUseColumnNames(&sessionProperties)); + EXPECT_EQ( + readerOptions.getFooterEstimatedSize(), + hiveConfig->footerEstimatedSize()); + EXPECT_EQ( + readerOptions.getFilePreloadThreshold(), + hiveConfig->filePreloadThreshold()); +} + +}; // namespace facebook::velox::connector diff --git a/velox/dwio/common/Options.h b/velox/dwio/common/Options.h index c154f8f4a6e0..92674f5a8fa2 100644 --- a/velox/dwio/common/Options.h +++ b/velox/dwio/common/Options.h @@ -93,7 +93,14 @@ class SerDeOptions { }; struct TableParameter { + /// If present in the table parameters, the option is passed to the row reader + /// to instruct it to skip the number of rows from the current position. Used + /// to skip the column header row(s). static constexpr const char* kSkipHeaderLineCount = "skip.header.line.count"; + /// If present in the table parameters, the option overrides the default value + /// of the SerDeOptions::nullString. It causes any field read from the file + /// (usually of the TEXT format) to be considered NULL if it is equal to this + /// string. static constexpr const char* kSerializationNullFormat = "serialization.null.format"; }; From aba702c05221e7ce8a64550542cc2791b6fb8d8a Mon Sep 17 00:00:00 2001 From: Zhenyuan Zhao Date: Tue, 13 Feb 2024 09:16:21 -0800 Subject: [PATCH 09/38] Return whether registration succeeded in the custom opaque path (#8716) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8716 This makes it easier for callers to check and decide whether they need to register serialization hooks Reviewed By: pedroerp Differential Revision: D53614803 fbshipit-source-id: a493bbf59ef6c82e5459279328ad7f530de86cbd --- velox/type/OpaqueCustomTypes.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/velox/type/OpaqueCustomTypes.h b/velox/type/OpaqueCustomTypes.h index fdb42d3d94a0..dacf0cfb87b7 100644 --- a/velox/type/OpaqueCustomTypes.h +++ b/velox/type/OpaqueCustomTypes.h @@ -33,8 +33,8 @@ class CastOperator; template class OpaqueCustomTypeRegister { public: - static void registerType() { - facebook::velox::registerCustomType( + static bool registerType() { + return facebook::velox::registerCustomType( customTypeName, std::make_unique()); } From b0eeef9177f3b79c010b243f9b2e4bd4d11120f3 Mon Sep 17 00:00:00 2001 From: "Schierbeck, Cody" Date: Tue, 13 Feb 2024 11:17:59 -0800 Subject: [PATCH 10/38] Introduce cappedByteLength to help with indexing UTF-8 strings (#8637) Summary: UTF strings may contain multi-byte characters that make character-based indexing inaccurate. This PR introduces functions stringImpl::cappedByteLength and stringCore::cappedByteLengthUnicode to help with indexing UTF strings that may contain multi-byte characters. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8637 Reviewed By: pedroerp Differential Revision: D53627624 Pulled By: kgpai fbshipit-source-id: 2f28a7d1bb81c1a5e875e7b8a6f300f1fc9fbb16 --- velox/functions/lib/string/StringCore.h | 27 ++++++ velox/functions/lib/string/StringImpl.h | 15 ++- .../lib/string/tests/StringImplTest.cpp | 94 +++++++++++++++++++ 3 files changed, 135 insertions(+), 1 deletion(-) diff --git a/velox/functions/lib/string/StringCore.h b/velox/functions/lib/string/StringCore.h index ba988b06c4af..8fdcc4c61892 100644 --- a/velox/functions/lib/string/StringCore.h +++ b/velox/functions/lib/string/StringCore.h @@ -264,6 +264,33 @@ cappedLengthUnicode(const char* input, size_t size, size_t maxChars) { return numChars; } +/// +/// Return an capped length in bytes(controlled by maxChars) of a unicode +/// string. The returned length may be greater than maxCharacters if there are +/// multi-byte characters present in the input string. +/// +/// This method is used to help with indexing unicode strings by byte position. +/// It is used to find the byte position of the Nth character in a string. +/// +/// @param input input buffer that hold the string +/// @param size size of input buffer +/// @param maxChars stop counting characters if the string is longer +/// than this value +/// @return the number of bytes represented by the input utf8 string up to +/// maxChars +/// +FOLLY_ALWAYS_INLINE int64_t +cappedByteLengthUnicode(const char* input, size_t size, int64_t maxChars) { + size_t utf8Position = 0; + size_t numCharacters = 0; + while (utf8Position < size && numCharacters < maxChars) { + auto charSize = utf8proc_char_length(input + utf8Position); + utf8Position += UNLIKELY(charSize < 0) ? 1 : charSize; + numCharacters++; + } + return utf8Position; +} + /// Returns the start byte index of the Nth instance of subString in /// string. Search starts from startPosition. Positions start with 0. If not /// found, -1 is returned. To facilitate finding overlapping strings, the diff --git a/velox/functions/lib/string/StringImpl.h b/velox/functions/lib/string/StringImpl.h index 871f3bffd194..73b6a4366162 100644 --- a/velox/functions/lib/string/StringImpl.h +++ b/velox/functions/lib/string/StringImpl.h @@ -111,7 +111,7 @@ FOLLY_ALWAYS_INLINE int64_t length(const T& input) { } } -/// Return a capped length(controlled by maxLength) of a string. +/// Return a capped length in characters(controlled by maxLength) of a string. /// The returned length is not greater than maxLength. template FOLLY_ALWAYS_INLINE int64_t cappedLength(const T& input, size_t maxLength) { @@ -122,6 +122,19 @@ FOLLY_ALWAYS_INLINE int64_t cappedLength(const T& input, size_t maxLength) { } } +/// Return a capped length in bytes(controlled by maxCharacters) of a string. +/// The returned length may be greater than maxCharacters if there are +/// multi-byte characters present in the input string. +template +FOLLY_ALWAYS_INLINE int64_t +cappedByteLength(const TString& input, size_t maxCharacters) { + if constexpr (isAscii) { + return input.size() > maxCharacters ? maxCharacters : input.size(); + } else { + return cappedByteLengthUnicode(input.data(), input.size(), maxCharacters); + } +} + /// Write the Unicode codePoint as string to the output string. The function /// behavior is undefined when code point it invalid. Implements the logic of /// presto chr function. diff --git a/velox/functions/lib/string/tests/StringImplTest.cpp b/velox/functions/lib/string/tests/StringImplTest.cpp index 258eb6f37053..883949e33c3a 100644 --- a/velox/functions/lib/string/tests/StringImplTest.cpp +++ b/velox/functions/lib/string/tests/StringImplTest.cpp @@ -196,6 +196,100 @@ TEST_F(StringImplTest, cappedLength) { ASSERT_EQ(cappedLength(input, 7), 5); } +TEST_F(StringImplTest, cappedUnicodeBytes) { + // Test functions use case for indexing + // UTF strings. + std::string stringInput = "\xF4\x90\x80\x80Hello"; + ASSERT_EQ('H', stringInput[cappedByteLength(stringInput, 2) - 1]); + ASSERT_EQ('e', stringInput[cappedByteLength(stringInput, 3) - 1]); + ASSERT_EQ('l', stringInput[cappedByteLength(stringInput, 4) - 1]); + ASSERT_EQ('l', stringInput[cappedByteLength(stringInput, 5) - 1]); + ASSERT_EQ('o', stringInput[cappedByteLength(stringInput, 6) - 1]); + ASSERT_EQ('o', stringInput[cappedByteLength(stringInput, 7) - 1]); + + // Multi-byte chars + stringInput = "♫¡Singing is fun!♫"; + auto sPos = cappedByteLength(stringInput, 2); + auto exPos = cappedByteLength(stringInput, 17); + ASSERT_EQ("Singing is fun!♫", stringInput.substr(sPos)); + ASSERT_EQ("♫¡Singing is fun!", stringInput.substr(0, exPos)); + ASSERT_EQ("Singing is fun!", stringInput.substr(sPos, exPos - sPos)); + + stringInput = std::string("abcd"); + auto stringViewInput = std::string_view(stringInput); + ASSERT_EQ(cappedByteLength(stringInput, 1), 1); + ASSERT_EQ(cappedByteLength(stringInput, 2), 2); + ASSERT_EQ(cappedByteLength(stringInput, 3), 3); + ASSERT_EQ(cappedByteLength(stringInput, 4), 4); + ASSERT_EQ(cappedByteLength(stringInput, 5), 4); + ASSERT_EQ(cappedByteLength(stringInput, 6), 4); + + ASSERT_EQ(cappedByteLength(stringViewInput, 1), 1); + ASSERT_EQ(cappedByteLength(stringViewInput, 2), 2); + ASSERT_EQ(cappedByteLength(stringViewInput, 3), 3); + ASSERT_EQ(cappedByteLength(stringViewInput, 4), 4); + ASSERT_EQ(cappedByteLength(stringViewInput, 5), 4); + ASSERT_EQ(cappedByteLength(stringViewInput, 6), 4); + + stringInput = std::string("你好a世界"); + stringViewInput = std::string_view(stringInput); + ASSERT_EQ(cappedByteLength(stringInput, 1), 3); + ASSERT_EQ(cappedByteLength(stringInput, 2), 6); + ASSERT_EQ(cappedByteLength(stringInput, 3), 7); + ASSERT_EQ(cappedByteLength(stringInput, 4), 10); + ASSERT_EQ(cappedByteLength(stringInput, 5), 13); + ASSERT_EQ(cappedByteLength(stringInput, 6), 13); + + ASSERT_EQ(cappedByteLength(stringViewInput, 1), 3); + ASSERT_EQ(cappedByteLength(stringViewInput, 2), 6); + ASSERT_EQ(cappedByteLength(stringViewInput, 3), 7); + ASSERT_EQ(cappedByteLength(stringViewInput, 4), 10); + ASSERT_EQ(cappedByteLength(stringViewInput, 5), 13); + ASSERT_EQ(cappedByteLength(stringViewInput, 6), 13); + + stringInput = std::string("\x80"); + stringViewInput = std::string_view(stringInput); + ASSERT_EQ(cappedByteLength(stringInput, 1), 1); + ASSERT_EQ(cappedByteLength(stringInput, 2), 1); + ASSERT_EQ(cappedByteLength(stringInput, 3), 1); + ASSERT_EQ(cappedByteLength(stringInput, 4), 1); + ASSERT_EQ(cappedByteLength(stringInput, 5), 1); + ASSERT_EQ(cappedByteLength(stringInput, 6), 1); + + ASSERT_EQ(cappedByteLength(stringViewInput, 1), 1); + ASSERT_EQ(cappedByteLength(stringViewInput, 2), 1); + ASSERT_EQ(cappedByteLength(stringViewInput, 3), 1); + ASSERT_EQ(cappedByteLength(stringViewInput, 4), 1); + ASSERT_EQ(cappedByteLength(stringViewInput, 5), 1); + ASSERT_EQ(cappedByteLength(stringViewInput, 6), 1); + + stringInput.resize(2); + // Create corrupt data below. + char16_t c = u'\u04FF'; + stringInput[0] = (char)c; + stringInput[1] = (char)c; + + ASSERT_EQ(cappedByteLength(stringInput, 1), 1); + + stringInput.resize(4); + c = u'\u04F4'; + char16_t c2 = u'\u048F'; + char16_t c3 = u'\u04BF'; + stringInput[0] = (char)c; + stringInput[1] = (char)c2; + stringInput[2] = (char)c3; + stringInput[3] = (char)c3; + + stringViewInput = std::string_view(stringInput); + ASSERT_EQ(cappedByteLength(stringInput, 1), 4); + ASSERT_EQ(cappedByteLength(stringInput, 2), 4); + ASSERT_EQ(cappedByteLength(stringInput, 3), 4); + + ASSERT_EQ(cappedByteLength(stringViewInput, 1), 4); + ASSERT_EQ(cappedByteLength(stringViewInput, 2), 4); + ASSERT_EQ(cappedByteLength(stringViewInput, 3), 4); +} + TEST_F(StringImplTest, badUnicodeLength) { ASSERT_EQ(0, length(std::string(""))); ASSERT_EQ(2, length(std::string("ab"))); From 76a5fd0ce2f01748e184b946ae39af93a8ae8f18 Mon Sep 17 00:00:00 2001 From: Bikramjeet Vig Date: Tue, 13 Feb 2024 15:26:54 -0800 Subject: [PATCH 11/38] Add FieldReference benchmark Summary: Adds a benchmark for FieldReference specifically for chained dereferences at different level. ``` ============================================================================ [...]hmarks/ExpressionBenchmarkBuilder.cpp relative time/iter iters/s ============================================================================ dereference_nullfree##1LevelThenFlat 275.83us 3.63K dereference_nullfree##1LevelThenComplex 419.07us 2.39K dereference_nullfree##2LevelThenFlat 431.62us 2.32K dereference_nullfree##2LevelThenComplex 599.98us 1.67K dereference_nullfree##3LevelThenFlat 632.73us 1.58K dereference_nullfree##3LevelThenComplex 777.87us 1.29K dereference_nullfree##4LevelThenFlat 746.93us 1.34K dereference_nullfree##4LevelThenComplex 867.19us 1.15K dereference_nulls##1LevelThenFlat 3.85ms 259.90 dereference_nulls##1LevelThenComplex 38.80ms 25.77 dereference_nulls##2LevelThenFlat 13.55ms 73.80 dereference_nulls##2LevelThenComplex 42.46ms 23.55 dereference_nulls##3LevelThenFlat 17.10ms 58.47 dereference_nulls##3LevelThenComplex 43.49ms 22.99 dereference_nulls##4LevelThenFlat 19.87ms 50.32 dereference_nulls##4LevelThenComplex 45.43ms 22.01 ``` Reviewed By: Yuhta Differential Revision: D53683303 fbshipit-source-id: 48b39c3dc4bcca6f2a4bd1249fd43fb8fc4e2492 --- .../prestosql/benchmarks/CMakeLists.txt | 5 ++ .../benchmarks/FieldReferenceBenchmark.cpp | 89 +++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 velox/functions/prestosql/benchmarks/FieldReferenceBenchmark.cpp diff --git a/velox/functions/prestosql/benchmarks/CMakeLists.txt b/velox/functions/prestosql/benchmarks/CMakeLists.txt index f7364c1d2287..e794460ef0d9 100644 --- a/velox/functions/prestosql/benchmarks/CMakeLists.txt +++ b/velox/functions/prestosql/benchmarks/CMakeLists.txt @@ -46,6 +46,11 @@ add_executable(velox_functions_prestosql_benchmarks_array_sum target_link_libraries(velox_functions_prestosql_benchmarks_array_sum ${BENCHMARK_DEPENDENCIES}) +add_executable(velox_functions_prestosql_benchmarks_field_reference + FieldReferenceBenchmark.cpp) +target_link_libraries(velox_functions_prestosql_benchmarks_field_reference + ${BENCHMARK_DEPENDENCIES}) + add_executable(velox_functions_prestosql_benchmarks_width_bucket WidthBucketBenchmark.cpp) target_link_libraries(velox_functions_prestosql_benchmarks_width_bucket diff --git a/velox/functions/prestosql/benchmarks/FieldReferenceBenchmark.cpp b/velox/functions/prestosql/benchmarks/FieldReferenceBenchmark.cpp new file mode 100644 index 000000000000..0abc546258ea --- /dev/null +++ b/velox/functions/prestosql/benchmarks/FieldReferenceBenchmark.cpp @@ -0,0 +1,89 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include +#include + +#include "velox/benchmarks/ExpressionBenchmarkBuilder.h" +#include "velox/functions/lib/benchmarks/FunctionBenchmarkBase.h" +#include "velox/functions/prestosql/registration/RegistrationFunctions.h" +#include "velox/vector/fuzzer/VectorFuzzer.h" + +using namespace facebook::velox; +using namespace facebook::velox::exec; +using namespace facebook::velox::functions; + +std::vector getColumnNames(int children) { + std::vector result; + for (int i = 0; i < children; ++i) { + result.push_back(fmt::format("{}{}", 'c', i)); + } + return result; +} + +RowTypePtr getRowColumnType(FuzzerGenerator& rng, int children, int level) { + VELOX_CHECK_GE(level, 1); + VELOX_CHECK_GE(children, 3); + std::vector result; + result.push_back(ARRAY(INTEGER())); + result.push_back(INTEGER()); + if (level > 1) { + result.push_back(getRowColumnType(rng, children, level - 1)); + } else { + result.push_back(randType(rng, 2)); + } + for (int i = 0; i < children - 3; ++i) { + result.push_back(randType(rng, 2)); + } + return ROW(getColumnNames(children), std::move(result)); +} + +int main(int argc, char** argv) { + folly::Init init{&argc, &argv}; + + ExpressionBenchmarkBuilder benchmarkBuilder; + FuzzerGenerator rng; + + auto createSet = [&](bool withNulls, RowTypePtr& inputType) { + benchmarkBuilder + .addBenchmarkSet( + fmt::format("dereference_{}", withNulls ? "nulls" : "nullfree"), + inputType) + .withFuzzerOptions( + {.vectorSize = 1000, .nullRatio = withNulls ? 0.2 : 0}) + .addExpression("1LevelThenFlat", "(c0).c1") + .addExpression("1LevelThenComplex", "(c0).c0") + .addExpression("2LevelThenFlat", "(c0).c2.c1") + .addExpression("2LevelThenComplex", "(c0).c2.c0") + .addExpression("3LevelThenFlat", "(c0).c2.c2.c1") + .addExpression("3LevelThenComplex", "(c0).c2.c2.c0") + .addExpression("4LevelThenFlat", "(c0).c2.c2.c2.c1") + .addExpression("4LevelThenComplex", "(c0).c2.c2.c2.c0"); + }; + + // Create a nested row column of depth 4. Each level has 50 columns. Each ROW + // at depth n will have the first three columns as ARRAY(INTEGER()), INTEGER() + // and ROW {of depth 4-n} respectively. The third column for the deepest ROW + // however can be anything. + auto inputType = ROW({"c0"}, {getRowColumnType(rng, 50, 4)}); + + createSet(true, inputType); + createSet(false, inputType); + + benchmarkBuilder.registerBenchmarks(); + + folly::runBenchmarks(); + return 0; +} From f0583e76be95d865074ccd3ad04343b9f03e3d2f Mon Sep 17 00:00:00 2001 From: Pedro Pedreira Date: Tue, 13 Feb 2024 15:49:47 -0800 Subject: [PATCH 12/38] Add `VELOX_BUILD_MINIMAL_WITH_DWIO` compilation option (#8682) Summary: `VELOX_BUILD_MINIMAL_WITH_DWIO` allows developers using Velox to compile only dwio (in addition to Velox minimal), but without pulling all other dependencies and internal libraries (exec, connectors, parser, aggregates, storage adapters, etc). Pull Request resolved: https://github.com/facebookincubator/velox/pull/8682 Reviewed By: Yuhta Differential Revision: D53729171 Pulled By: pedroerp fbshipit-source-id: 417e8cc4fc2b512ec658fe76a38477d7e30026f8 --- CMakeLists.txt | 23 ++++++++++++++++++----- Makefile | 18 +++++++++++++++++- velox/CMakeLists.txt | 4 ++-- velox/codegen/CMakeLists.txt | 2 +- velox/dwio/common/CMakeLists.txt | 1 - 5 files changed, 38 insertions(+), 10 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c7dc7d568d3..e2099787a969 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -62,6 +62,11 @@ option( VELOX_BUILD_MINIMAL "Build a minimal set of components only. This will override other build options." OFF) +option( + VELOX_BUILD_MINIMAL_WITH_DWIO + "Build a minimal set of components, including DWIO (file format readers/writers). + This will override other build options." + OFF) # option() always creates a BOOL variable so we have to use a normal cache # variable with STRING type for this option. @@ -96,6 +101,7 @@ option(VELOX_ENABLE_PARQUET "Enable Parquet support" OFF) option(VELOX_ENABLE_ARROW "Enable Arrow support" OFF) option(VELOX_ENABLE_REMOTE_FUNCTIONS "Enable remote function support" OFF) option(VELOX_ENABLE_CCACHE "Use ccache if installed." ON) +option(VELOX_ENABLE_CODEGEN_SUPPORT "Enable experimental codegen support." OFF) option(VELOX_BUILD_TEST_UTILS "Builds Velox test utilities" OFF) option(VELOX_BUILD_PYTHON_PACKAGE "Builds Velox Python bindings" OFF) @@ -125,7 +131,7 @@ if(${VELOX_BUILD_MINIMAL}) set(VELOX_ENABLE_GCS OFF) set(VELOX_ENABLE_ABFS OFF) set(VELOX_ENABLE_SUBSTRAIT OFF) - set(VELOX_CODEGEN_SUPPORT OFF) + set(VELOX_ENABLE_CODEGEN_SUPPORT OFF) endif() if(${VELOX_BUILD_TESTING}) @@ -175,7 +181,7 @@ if(${VELOX_BUILD_PYTHON_PACKAGE}) set(VELOX_ENABLE_GCS OFF) set(VELOX_ENABLE_ABFS OFF) set(VELOX_ENABLE_SUBSTRAIT OFF) - set(VELOX_CODEGEN_SUPPORT OFF) + set(VELOX_ENABLE_CODEGEN_SUPPORT OFF) set(VELOX_ENABLE_BENCHMARKS_BASIC OFF) set(VELOX_ENABLE_BENCHMARKS OFF) endif() @@ -257,7 +263,7 @@ if(VELOX_ENABLE_PARQUET) endif() # define processor variable for conditional compilation -if(${VELOX_CODEGEN_SUPPORT}) +if(${VELOX_ENABLE_CODEGEN_SUPPORT}) add_compile_definitions(CODEGEN_ENABLED=1) endif() @@ -420,7 +426,10 @@ endif() set_source(fmt) resolve_dependency(fmt 9.0.0) -if(NOT ${VELOX_BUILD_MINIMAL}) +if(${VELOX_BUILD_MINIMAL_WITH_DWIO} OR ${VELOX_ENABLE_HIVE_CONNECTOR}) + # DWIO needs all sorts of stream compression libraries. + # + # TODO: make these optional and pluggable. find_package(ZLIB REQUIRED) find_package(lz4 REQUIRED) find_package(lzo2 REQUIRED) @@ -467,7 +476,11 @@ else() set(FOLLY_BENCHMARK Folly::follybenchmark) endif() -if(NOT ${VELOX_BUILD_MINIMAL}) +# DWIO (ORC/DWRF), Substrait and experimental/codegen depend on protobuf. +if(${VELOX_BUILD_MINIMAL_WITH_DWIO} + OR ${VELOX_ENABLE_HIVE_CONNECTOR} + OR ${VELOX_ENABLE_SUBSTRAIT} + OR ${VELOX_ENABLE_CODEGEN_SUPPORT}) # Locate or build protobuf. set_source(Protobuf) resolve_dependency(Protobuf 3.21 EXACT) diff --git a/Makefile b/Makefile index 794d876b41c9..82de59432436 100644 --- a/Makefile +++ b/Makefile @@ -98,10 +98,26 @@ release: #: Build the release version $(MAKE) cmake BUILD_DIR=release BUILD_TYPE=Release && \ $(MAKE) build BUILD_DIR=release -min_debug: #: Minimal build with debugging symbols +minimal_debug: #: Minimal build with debugging symbols $(MAKE) cmake BUILD_DIR=debug BUILD_TYPE=debug EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} -DVELOX_BUILD_MINIMAL=ON" $(MAKE) build BUILD_DIR=debug +min_debug: minimal_debug + +minimal: #: Minimal build + $(MAKE) cmake BUILD_DIR=release BUILD_TYPE=release EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} -DVELOX_BUILD_MINIMAL=ON" + $(MAKE) build BUILD_DIR=release + +dwio: #: Minimal build with dwio enabled. + $(MAKE) cmake BUILD_DIR=release BUILD_TYPE=release EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} \ + -DVELOX_BUILD_MINIMAL_WITH_DWIO=ON" + $(MAKE) build BUILD_DIR=release + +dwio_debug: #: Minimal build with dwio debugging symbols. + $(MAKE) cmake BUILD_DIR=debug BUILD_TYPE=debug EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} \ + -DVELOX_BUILD_MINIMAL_WITH_DWIO=ON" + $(MAKE) build BUILD_DIR=debug + benchmarks-basic-build: $(MAKE) release EXTRA_CMAKE_FLAGS=" ${EXTRA_CMAKE_FLAGS} \ -DVELOX_BUILD_TESTING=OFF \ diff --git a/velox/CMakeLists.txt b/velox/CMakeLists.txt index cd18d344e809..be53ad8beeb9 100644 --- a/velox/CMakeLists.txt +++ b/velox/CMakeLists.txt @@ -44,7 +44,7 @@ if(${VELOX_ENABLE_PARSE}) endif() # hive connector depends on dwio -if(${VELOX_ENABLE_HIVE_CONNECTOR}) +if(${VELOX_BUILD_MINIMAL_WITH_DWIO} OR ${VELOX_ENABLE_HIVE_CONNECTOR}) add_subdirectory(dwio) endif() @@ -65,7 +65,7 @@ if(${VELOX_ENABLE_DUCKDB}) add_subdirectory(duckdb) endif() -if(${VELOX_CODEGEN_SUPPORT}) +if(${VELOX_ENABLE_CODEGEN_SUPPORT}) add_subdirectory(experimental/codegen) endif() diff --git a/velox/codegen/CMakeLists.txt b/velox/codegen/CMakeLists.txt index e54a0d133c07..37e7d2a34389 100644 --- a/velox/codegen/CMakeLists.txt +++ b/velox/codegen/CMakeLists.txt @@ -13,7 +13,7 @@ # limitations under the License. add_library(velox_codegen Codegen.cpp) -if(${VELOX_CODEGEN_SUPPORT}) +if(${VELOX_ENABLE_CODEGEN_SUPPORT}) target_link_libraries(velox_codegen velox_experimental_codegen) else() target_link_libraries(velox_codegen velox_core velox_exec velox_expression diff --git a/velox/dwio/common/CMakeLists.txt b/velox/dwio/common/CMakeLists.txt index 8334de75e0f5..25e8bb56a104 100644 --- a/velox/dwio/common/CMakeLists.txt +++ b/velox/dwio/common/CMakeLists.txt @@ -74,7 +74,6 @@ target_link_libraries( velox_exception velox_expression velox_memory - velox_exec Boost::regex Folly::folly glog::glog) From dec4c446f92806fb4a3671cd19da81526a695ff8 Mon Sep 17 00:00:00 2001 From: Masha Basmanova Date: Tue, 13 Feb 2024 16:18:19 -0800 Subject: [PATCH 13/38] Fix 'out of range in dynamic array' error in Task::toJson (#8735) Summary: Task::toJson used to create folly::dynamic::array for drivers and access non-existing elements via [index]. That resulted in 'out of range in dynamic array' errors. Fix is to use folly::dynamic::object. Fixes https://github.com/prestodb/presto/issues/21917 Pull Request resolved: https://github.com/facebookincubator/velox/pull/8735 Reviewed By: spershin Differential Revision: D53727317 Pulled By: mbasmanova fbshipit-source-id: afcfd490fd44d67b78c0b70b3f557a179fddd123 --- velox/exec/Task.cpp | 11 +++++---- velox/exec/tests/TaskTest.cpp | 42 +++++++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/velox/exec/Task.cpp b/velox/exec/Task.cpp index fd5addd7af67..364ba829fb4e 100644 --- a/velox/exec/Task.cpp +++ b/velox/exec/Task.cpp @@ -2154,14 +2154,13 @@ folly::dynamic Task::toJson() const { obj["plan"] = planFragment_.planNode->toString(true, true); } - folly::dynamic driverObj = folly::dynamic::array; - int index = 0; - for (auto& driver : drivers_) { - if (driver) { - driverObj[index++] = driver->toJson(); + folly::dynamic drivers = folly::dynamic::object; + for (auto i = 0; i < drivers_.size(); ++i) { + if (drivers_[i] != nullptr) { + drivers[i] = drivers_[i]->toJson(); } } - obj["drivers"] = driverObj; + obj["drivers"] = drivers; if (auto buffers = bufferManager_.lock()) { if (auto buffer = buffers->getBufferIfExists(taskId_)) { diff --git a/velox/exec/tests/TaskTest.cpp b/velox/exec/tests/TaskTest.cpp index c64252719084..37c9e5618395 100644 --- a/velox/exec/tests/TaskTest.cpp +++ b/velox/exec/tests/TaskTest.cpp @@ -502,14 +502,7 @@ class TaskTest : public HiveConnectorTestBase { } }; -TEST_F(TaskTest, wrongPlanNodeForSplit) { - auto connectorSplit = std::make_shared( - "test", - "file:/tmp/abc", - facebook::velox::dwio::common::FileFormat::DWRF, - 0, - 100); - +TEST_F(TaskTest, toJson) { auto plan = PlanBuilder() .tableScan(ROW({"a", "b"}, {INTEGER(), DOUBLE()})) .project({"a * a", "b + b"}) @@ -525,11 +518,42 @@ TEST_F(TaskTest, wrongPlanNodeForSplit) { task->toString(), "{Task task-1 (task-1)Plan: -- Project\n\n drivers:\n"); ASSERT_EQ( folly::toPrettyJson(task->toJson()), - "{\n \"concurrentSplitGroups\": 1,\n \"drivers\": [],\n \"exchangeClientByPlanNode\": {},\n \"groupedPartitionedOutput\": false,\n \"id\": \"task-1\",\n \"noMoreOutputBuffers\": false,\n \"numDriversPerSplitGroup\": 0,\n \"numDriversUngrouped\": 0,\n \"numFinishedDrivers\": 0,\n \"numRunningDrivers\": 0,\n \"numRunningSplitGroups\": 0,\n \"numThreads\": 0,\n \"numTotalDrivers\": 0,\n \"onThreadSince\": \"0\",\n \"partitionedOutputConsumed\": false,\n \"pauseRequested\": false,\n \"plan\": \"-- Project[expressions: (p0:INTEGER, multiply(ROW[\\\"a\\\"],ROW[\\\"a\\\"])), (p1:DOUBLE, plus(ROW[\\\"b\\\"],ROW[\\\"b\\\"]))] -> p0:INTEGER, p1:DOUBLE\\n -- TableScan[table: hive_table] -> a:INTEGER, b:DOUBLE\\n\",\n \"shortId\": \"task-1\",\n \"state\": \"Running\",\n \"terminateRequested\": false\n}"); + "{\n \"concurrentSplitGroups\": 1,\n \"drivers\": {},\n \"exchangeClientByPlanNode\": {},\n \"groupedPartitionedOutput\": false,\n \"id\": \"task-1\",\n \"noMoreOutputBuffers\": false,\n \"numDriversPerSplitGroup\": 0,\n \"numDriversUngrouped\": 0,\n \"numFinishedDrivers\": 0,\n \"numRunningDrivers\": 0,\n \"numRunningSplitGroups\": 0,\n \"numThreads\": 0,\n \"numTotalDrivers\": 0,\n \"onThreadSince\": \"0\",\n \"partitionedOutputConsumed\": false,\n \"pauseRequested\": false,\n \"plan\": \"-- Project[expressions: (p0:INTEGER, multiply(ROW[\\\"a\\\"],ROW[\\\"a\\\"])), (p1:DOUBLE, plus(ROW[\\\"b\\\"],ROW[\\\"b\\\"]))] -> p0:INTEGER, p1:DOUBLE\\n -- TableScan[table: hive_table] -> a:INTEGER, b:DOUBLE\\n\",\n \"shortId\": \"task-1\",\n \"state\": \"Running\",\n \"terminateRequested\": false\n}"); ASSERT_EQ( folly::toPrettyJson(task->toShortJson()), "{\n \"id\": \"task-1\",\n \"numFinishedDrivers\": 0,\n \"numRunningDrivers\": 0,\n \"numThreads\": 0,\n \"numTotalDrivers\": 0,\n \"pauseRequested\": false,\n \"shortId\": \"task-1\",\n \"state\": \"Running\",\n \"terminateRequested\": false\n}"); + task->start(2); + + ASSERT_NO_THROW(task->toJson()); + ASSERT_NO_THROW(task->toShortJson()); + + task->noMoreSplits("0"); + waitForTaskCompletion(task.get()); + + ASSERT_NO_THROW(task->toJson()); + ASSERT_NO_THROW(task->toShortJson()); +} + +TEST_F(TaskTest, wrongPlanNodeForSplit) { + auto connectorSplit = std::make_shared( + "test", + "file:/tmp/abc", + facebook::velox::dwio::common::FileFormat::DWRF, + 0, + 100); + + auto plan = PlanBuilder() + .tableScan(ROW({"a", "b"}, {INTEGER(), DOUBLE()})) + .project({"a * a", "b + b"}) + .planFragment(); + + auto task = Task::create( + "task-1", + std::move(plan), + 0, + std::make_shared(driverExecutor_.get())); + // Add split for the source node. task->addSplit("0", exec::Split(folly::copy(connectorSplit))); From 793222bf59a88f2539ce0e8d42d881e47e24511a Mon Sep 17 00:00:00 2001 From: Masha Basmanova Date: Tue, 13 Feb 2024 17:18:10 -0800 Subject: [PATCH 14/38] Report rawInputPositions stat for MergeExchange (#8742) Summary: Source operators are expected to report rawInputBytes and rawInputPositions. MergeExchange didn't report rawInputPositions. Prestissimo uses 'rawInputPositions' to show number of rows processed by a task in the coordinator UI. When this stat is missing, the UI shows zero. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8742 Reviewed By: Yuhta Differential Revision: D53730491 Pulled By: mbasmanova fbshipit-source-id: 7747f57521549e33e4a297535c5f3170c65512a2 --- velox/exec/MergeSource.cpp | 1 + velox/exec/tests/MultiFragmentTest.cpp | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/velox/exec/MergeSource.cpp b/velox/exec/MergeSource.cpp index 36904506f8bd..08bb79b8acac 100644 --- a/velox/exec/MergeSource.cpp +++ b/velox/exec/MergeSource.cpp @@ -167,6 +167,7 @@ class MergeExchangeSource : public MergeSource { auto lockedStats = mergeExchange_->stats().wlock(); lockedStats->addInputVector(data->estimateFlatSize(), data->size()); + lockedStats->rawInputPositions += data->size(); } // Since VectorStreamGroup::read() may cause inputStream to be at end, diff --git a/velox/exec/tests/MultiFragmentTest.cpp b/velox/exec/tests/MultiFragmentTest.cpp index 52f539138dea..32957ad8154e 100644 --- a/velox/exec/tests/MultiFragmentTest.cpp +++ b/velox/exec/tests/MultiFragmentTest.cpp @@ -404,8 +404,10 @@ TEST_F(MultiFragmentTest, mergeExchange) { } auto finalSortTaskId = makeTaskId("orderby", tasks.size()); + core::PlanNodeId mergeExchangeId; auto finalSortPlan = PlanBuilder() .mergeExchange(outputType, {"c0"}) + .capturePlanNodeId(mergeExchangeId) .partitionedOutput({}, 1) .planNode(); @@ -421,6 +423,15 @@ TEST_F(MultiFragmentTest, mergeExchange) { for (auto& task : tasks) { ASSERT_TRUE(waitForTaskCompletion(task.get())) << task->taskId(); } + + const auto finalSortStats = toPlanStats(task->taskStats()); + const auto& mergeExchangeStats = finalSortStats.at(mergeExchangeId); + + EXPECT_EQ(20'000, mergeExchangeStats.inputRows); + EXPECT_EQ(20'000, mergeExchangeStats.rawInputRows); + + EXPECT_LT(0, mergeExchangeStats.inputBytes); + EXPECT_LT(0, mergeExchangeStats.rawInputBytes); } // Test reordering and dropping columns in PartitionedOutput operator. From 5d1d2a3fa5055b1ff14f2b5154cfd9570ce35984 Mon Sep 17 00:00:00 2001 From: Ankita Victor Date: Tue, 13 Feb 2024 17:20:26 -0800 Subject: [PATCH 15/38] Fix install_conda in setup-ubuntu.sh to consider the CPU architecture (#8706) Summary: The function install_conda in setup_ubuntu.sh always downloads the conda package corresponding to the CPU architecture x86_64. Fix the function to download the conda package based on the actual CPU architecture (x86_64 or aarch64). Resolves https://github.com/facebookincubator/velox/issues/8453 Pull Request resolved: https://github.com/facebookincubator/velox/pull/8706 Reviewed By: Yuhta Differential Revision: D53727378 Pulled By: mbasmanova fbshipit-source-id: 5e2e076dd3a67f3de115dcc64c682b4cbf59fd34 --- scripts/setup-ubuntu.sh | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh index 69760cf85ec0..60b37c3b2ad5 100755 --- a/scripts/setup-ubuntu.sh +++ b/scripts/setup-ubuntu.sh @@ -119,11 +119,20 @@ function install_fbthrift { function install_conda { mkdir -p conda && cd conda - wget https://repo.anaconda.com/miniconda/Miniconda3-latest-Linux-x86_64.sh + ARCH=$(uname -m) + + if [ "$ARCH" != "x86_64" ] && [ "$ARCH" != "aarch64" ]; then + echo "Unsupported architecture: $ARCH" + exit 1 + fi + + wget https://repo.anaconda.com/miniconda/Miniconda3-latest-Linux-$ARCH.sh + MINICONDA_PATH=/opt/miniconda-for-velox - bash Miniconda3-latest-Linux-x86_64.sh -b -p $MINICONDA_PATH + bash Miniconda3-latest-Linux-$ARCH.sh -b -p $MINICONDA_PATH } + function install_velox_deps { run_and_time install_fmt run_and_time install_folly From 701c95da4c13ec88dcfac3e9780f09bb9386a5b5 Mon Sep 17 00:00:00 2001 From: Masha Basmanova Date: Wed, 14 Feb 2024 07:11:22 -0800 Subject: [PATCH 16/38] Fix task id used to create ExchangeClient for MergeExchangeSource (#8743) Summary: ExchangeClient's constructor takes task ID of the owning task, the task that receives and processes the data coming from the exchange. MergeExchangeSource used to create ExchangeClient using task ID of the remote task, the task the data is being pulled from. ExchangeClient uses task ID only for logging, hence, there is no hard failure when using the wrong task ID. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8743 Reviewed By: Yuhta Differential Revision: D53741039 Pulled By: mbasmanova fbshipit-source-id: d2213fe5330c28e9ec98261bbd86a4aba8c4f58f --- velox/exec/MergeSource.cpp | 2 +- velox/exec/Operator.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/velox/exec/MergeSource.cpp b/velox/exec/MergeSource.cpp index 08bb79b8acac..0ba8b6463d69 100644 --- a/velox/exec/MergeSource.cpp +++ b/velox/exec/MergeSource.cpp @@ -125,7 +125,7 @@ class MergeExchangeSource : public MergeSource { folly::Executor* executor) : mergeExchange_(mergeExchange), client_(std::make_shared( - taskId, + mergeExchange->taskId(), destination, maxQueuedBytes, pool, diff --git a/velox/exec/Operator.h b/velox/exec/Operator.h index 189f209dd864..030e06fbc764 100644 --- a/velox/exec/Operator.h +++ b/velox/exec/Operator.h @@ -519,6 +519,10 @@ class Operator : public BaseRuntimeStatWriter { return operatorCtx_->operatorType(); } + const std::string& taskId() const { + return operatorCtx_->taskId(); + } + /// Registers 'translator' for mapping user defined PlanNode subclass /// instances to user-defined Operators. static void registerOperator(std::unique_ptr translator); From 0fabd2463ee818b64d8bcf165089f63eb7d7ae43 Mon Sep 17 00:00:00 2001 From: Ankita Victor Date: Wed, 14 Feb 2024 07:12:54 -0800 Subject: [PATCH 17/38] Drop support for TIMESTAMP input from dayofweek Spark function (#8746) Summary: In Spark `dayofweek` doesn't directly accept TIMESTAMP input. For TIMESTAMP input, Spark will add a cast expression to convert it to date type, so only DATE type can be considered in Velox. Also removing alias `dow`. Spark function doc - https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.functions.dayofweek.html Addresses https://github.com/facebookincubator/velox/issues/8736 Pull Request resolved: https://github.com/facebookincubator/velox/pull/8746 Reviewed By: Yuhta Differential Revision: D53748021 Pulled By: mbasmanova fbshipit-source-id: cee7f7a7a216085485f043451616da508a2cec58 --- velox/docs/functions/spark/datetime.rst | 11 +-- velox/functions/sparksql/DateTimeFunctions.h | 8 +- velox/functions/sparksql/Register.cpp | 5 +- .../sparksql/tests/DateTimeFunctionsTest.cpp | 84 +++++-------------- 4 files changed, 24 insertions(+), 84 deletions(-) diff --git a/velox/docs/functions/spark/datetime.rst b/velox/docs/functions/spark/datetime.rst index 3ac392536524..bcdae4c2c637 100644 --- a/velox/docs/functions/spark/datetime.rst +++ b/velox/docs/functions/spark/datetime.rst @@ -71,17 +71,12 @@ These functions support TIMESTAMP and DATE input types. SELECT dayofyear('2016-04-09'); -- 100 -.. spark:function:: dayofweek(date/timestamp) -> integer +.. spark:function:: dayofweek(date) -> integer - Returns the day of the week for date/timestamp (1 = Sunday, 2 = Monday, ..., 7 = Saturday). - We can use `dow` as alias for :: + Returns the day of the week for date (1 = Sunday, 2 = Monday, ..., 7 = Saturday). SELECT dayofweek('2009-07-30'); -- 5 - SELECT dayofweek('2023-08-22 11:23:00.100'); -- 3 - -.. spark:function:: dow(x) -> integer - - This is an alias for :spark:func:`dayofweek`. + SELECT dayofweek('2023-08-22'); -- 3 .. spark:function:: from_unixtime(unixTime, format) -> string diff --git a/velox/functions/sparksql/DateTimeFunctions.h b/velox/functions/sparksql/DateTimeFunctions.h index 94a7f21317e5..f2b452f7cb7d 100644 --- a/velox/functions/sparksql/DateTimeFunctions.h +++ b/velox/functions/sparksql/DateTimeFunctions.h @@ -396,7 +396,7 @@ struct DateSubFunction { }; template -struct DayOfWeekFunction : public InitSessionTimezone { +struct DayOfWeekFunction { VELOX_DEFINE_FUNCTION_TYPES(T); // 1 = Sunday, 2 = Monday, ..., 7 = Saturday @@ -404,12 +404,6 @@ struct DayOfWeekFunction : public InitSessionTimezone { return time.tm_wday + 1; } - FOLLY_ALWAYS_INLINE void call( - int32_t& result, - const arg_type& timestamp) { - result = getDayOfWeek(getDateTime(timestamp, this->timeZone_)); - } - FOLLY_ALWAYS_INLINE void call(int32_t& result, const arg_type& date) { result = getDayOfWeek(getDateTime(date)); } diff --git a/velox/functions/sparksql/Register.cpp b/velox/functions/sparksql/Register.cpp index a24c0bf215ae..cf31e9512d74 100644 --- a/velox/functions/sparksql/Register.cpp +++ b/velox/functions/sparksql/Register.cpp @@ -304,10 +304,7 @@ void registerFunctions(const std::string& prefix) { registerFunction( {prefix + "doy", prefix + "dayofyear"}); - registerFunction( - {prefix + "dow", prefix + "dayofweek"}); - registerFunction( - {prefix + "dow", prefix + "dayofweek"}); + registerFunction({prefix + "dayofweek"}); registerFunction({prefix + "quarter"}); diff --git a/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp b/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp index b7cfbe4d694e..df0067060b71 100644 --- a/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp +++ b/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp @@ -427,73 +427,27 @@ TEST_F(DateTimeFunctionsTest, dayOfMonth) { } TEST_F(DateTimeFunctionsTest, dayOfWeekDate) { - const auto dayOfWeek = [&](std::optional date, - const std::string& func) { - return evaluateOnce( - fmt::format("{}(c0)", func), {date}, {DATE()}); + const auto dayOfWeek = [&](std::optional date) { + return evaluateOnce("dayofweek(c0)", {date}, {DATE()}); }; - for (const auto& func : {"dayofweek", "dow"}) { - EXPECT_EQ(std::nullopt, dayOfWeek(std::nullopt, func)); - EXPECT_EQ(5, dayOfWeek(0, func)); - EXPECT_EQ(4, dayOfWeek(-1, func)); - EXPECT_EQ(7, dayOfWeek(-40, func)); - EXPECT_EQ(5, dayOfWeek(parseDate("2009-07-30"), func)); - EXPECT_EQ(1, dayOfWeek(parseDate("2023-08-20"), func)); - EXPECT_EQ(2, dayOfWeek(parseDate("2023-08-21"), func)); - EXPECT_EQ(3, dayOfWeek(parseDate("2023-08-22"), func)); - EXPECT_EQ(4, dayOfWeek(parseDate("2023-08-23"), func)); - EXPECT_EQ(5, dayOfWeek(parseDate("2023-08-24"), func)); - EXPECT_EQ(6, dayOfWeek(parseDate("2023-08-25"), func)); - EXPECT_EQ(7, dayOfWeek(parseDate("2023-08-26"), func)); - EXPECT_EQ(1, dayOfWeek(parseDate("2023-08-27"), func)); - - // test cases from spark's DateExpressionSuite. - EXPECT_EQ(6, dayOfWeek(util::fromDateString("2011-05-06"), func)); - } -} - -TEST_F(DateTimeFunctionsTest, dayofWeekTs) { - const auto dayOfWeek = [&](std::optional date, - const std::string& func) { - return evaluateOnce(fmt::format("{}(c0)", func), date); - }; - - for (const auto& func : {"dayofweek", "dow"}) { - EXPECT_EQ(5, dayOfWeek(Timestamp(0, 0), func)); - EXPECT_EQ(4, dayOfWeek(Timestamp(-1, 0), func)); - EXPECT_EQ( - 1, - dayOfWeek(util::fromTimestampString("2023-08-20 20:23:00.001"), func)); - EXPECT_EQ( - 2, - dayOfWeek(util::fromTimestampString("2023-08-21 21:23:00.030"), func)); - EXPECT_EQ( - 3, - dayOfWeek(util::fromTimestampString("2023-08-22 11:23:00.100"), func)); - EXPECT_EQ( - 4, - dayOfWeek(util::fromTimestampString("2023-08-23 22:23:00.030"), func)); - EXPECT_EQ( - 5, - dayOfWeek(util::fromTimestampString("2023-08-24 15:23:00.000"), func)); - EXPECT_EQ( - 6, - dayOfWeek(util::fromTimestampString("2023-08-25 03:23:04.000"), func)); - EXPECT_EQ( - 7, - dayOfWeek(util::fromTimestampString("2023-08-26 01:03:00.300"), func)); - EXPECT_EQ( - 1, - dayOfWeek(util::fromTimestampString("2023-08-27 01:13:00.000"), func)); - // test cases from spark's DateExpressionSuite. - EXPECT_EQ( - 4, dayOfWeek(util::fromTimestampString("2015-04-08 13:10:15"), func)); - EXPECT_EQ( - 7, dayOfWeek(util::fromTimestampString("2017-05-27 13:10:15"), func)); - EXPECT_EQ( - 6, dayOfWeek(util::fromTimestampString("1582-10-15 13:10:15"), func)); - } + EXPECT_EQ(std::nullopt, dayOfWeek(std::nullopt)); + EXPECT_EQ(5, dayOfWeek(0)); + EXPECT_EQ(4, dayOfWeek(-1)); + EXPECT_EQ(7, dayOfWeek(-40)); + EXPECT_EQ(5, dayOfWeek(parseDate("2009-07-30"))); + EXPECT_EQ(1, dayOfWeek(parseDate("2023-08-20"))); + EXPECT_EQ(2, dayOfWeek(parseDate("2023-08-21"))); + EXPECT_EQ(3, dayOfWeek(parseDate("2023-08-22"))); + EXPECT_EQ(4, dayOfWeek(parseDate("2023-08-23"))); + EXPECT_EQ(5, dayOfWeek(parseDate("2023-08-24"))); + EXPECT_EQ(6, dayOfWeek(parseDate("2023-08-25"))); + EXPECT_EQ(7, dayOfWeek(parseDate("2023-08-26"))); + EXPECT_EQ(1, dayOfWeek(parseDate("2023-08-27"))); + EXPECT_EQ(6, dayOfWeek(util::fromDateString("2011-05-06"))); + EXPECT_EQ(4, dayOfWeek(util::fromDateString("2015-04-08"))); + EXPECT_EQ(7, dayOfWeek(util::fromDateString("2017-05-27"))); + EXPECT_EQ(6, dayOfWeek(util::fromDateString("1582-10-15"))); } TEST_F(DateTimeFunctionsTest, dateDiffDate) { From afe819e807100262c8b056efa3defb29606daf6e Mon Sep 17 00:00:00 2001 From: yingsu00 Date: Wed, 14 Feb 2024 07:26:14 -0800 Subject: [PATCH 18/38] Support reading Iceberg positional delete files (#7847) Summary: In this PR we introduces IcebergSplitReader which supports reading Iceberg splits with positional delete files Pull Request resolved: https://github.com/facebookincubator/velox/pull/7847 Reviewed By: mbasmanova Differential Revision: D53591413 Pulled By: Yuhta fbshipit-source-id: 8b8b5c0487ae1d1ecc24e0ca5def6d62df34eee1 --- velox/connectors/hive/CMakeLists.txt | 30 +- velox/connectors/hive/FileHandle.h | 5 - velox/connectors/hive/HiveConnectorUtil.cpp | 22 +- velox/connectors/hive/HiveConnectorUtil.h | 8 + velox/connectors/hive/SplitReader.cpp | 40 ++- velox/connectors/hive/iceberg/CMakeLists.txt | 28 ++ .../hive/iceberg/IcebergDeleteFile.h | 69 +++++ .../hive/iceberg/IcebergMetadataColumns.h | 55 ++++ .../connectors/hive/iceberg/IcebergSplit.cpp | 69 +++++ velox/connectors/hive/iceberg/IcebergSplit.h | 56 ++++ .../hive/iceberg/IcebergSplitReader.cpp | 113 +++++++ .../hive/iceberg/IcebergSplitReader.h | 62 ++++ .../iceberg/PositionalDeleteFileReader.cpp | 243 +++++++++++++++ .../hive/iceberg/PositionalDeleteFileReader.h | 86 ++++++ .../hive/iceberg/tests/CMakeLists.txt | 34 +++ .../hive/iceberg/tests/IcebergReadTest.cpp | 280 ++++++++++++++++++ 16 files changed, 1167 insertions(+), 33 deletions(-) create mode 100644 velox/connectors/hive/iceberg/CMakeLists.txt create mode 100644 velox/connectors/hive/iceberg/IcebergDeleteFile.h create mode 100644 velox/connectors/hive/iceberg/IcebergMetadataColumns.h create mode 100644 velox/connectors/hive/iceberg/IcebergSplit.cpp create mode 100644 velox/connectors/hive/iceberg/IcebergSplit.h create mode 100644 velox/connectors/hive/iceberg/IcebergSplitReader.cpp create mode 100644 velox/connectors/hive/iceberg/IcebergSplitReader.h create mode 100644 velox/connectors/hive/iceberg/PositionalDeleteFileReader.cpp create mode 100644 velox/connectors/hive/iceberg/PositionalDeleteFileReader.h create mode 100644 velox/connectors/hive/iceberg/tests/CMakeLists.txt create mode 100644 velox/connectors/hive/iceberg/tests/IcebergReadTest.cpp diff --git a/velox/connectors/hive/CMakeLists.txt b/velox/connectors/hive/CMakeLists.txt index 265a35b54f80..f8f60c41c4e2 100644 --- a/velox/connectors/hive/CMakeLists.txt +++ b/velox/connectors/hive/CMakeLists.txt @@ -13,9 +13,10 @@ # limitations under the License. add_library(velox_hive_config OBJECT HiveConfig.cpp) - target_link_libraries(velox_hive_config velox_exception) +add_subdirectory(iceberg) + add_library( velox_hive_connector OBJECT FileHandle.cpp @@ -31,19 +32,20 @@ add_library( target_link_libraries( velox_hive_connector - velox_common_io - velox_connector - velox_dwio_catalog_fbhive - velox_dwio_dwrf_reader - velox_dwio_dwrf_writer - velox_dwio_parquet_reader - velox_dwio_parquet_writer - velox_file - velox_hive_partition_function - velox_s3fs - velox_hdfs - velox_gcs - velox_abfs) + PUBLIC velox_hive_iceberg_splitreader + PRIVATE velox_common_io + velox_connector + velox_dwio_catalog_fbhive + velox_dwio_dwrf_reader + velox_dwio_dwrf_writer + velox_dwio_parquet_reader + velox_dwio_parquet_writer + velox_file + velox_hive_partition_function + velox_s3fs + velox_hdfs + velox_gcs + velox_abfs) add_library(velox_hive_partition_function HivePartitionFunction.cpp) diff --git a/velox/connectors/hive/FileHandle.h b/velox/connectors/hive/FileHandle.h index 15edd9d2ac2f..6fb6853d7544 100644 --- a/velox/connectors/hive/FileHandle.h +++ b/velox/connectors/hive/FileHandle.h @@ -25,14 +25,9 @@ #pragma once -#include -#include -#include - #include "velox/common/caching/CachedFactory.h" #include "velox/common/caching/FileIds.h" #include "velox/common/file/File.h" -#include "velox/dwio/common/InputStream.h" namespace facebook::velox { diff --git a/velox/connectors/hive/HiveConnectorUtil.cpp b/velox/connectors/hive/HiveConnectorUtil.cpp index a0bc7fbd046e..95b47de314a0 100644 --- a/velox/connectors/hive/HiveConnectorUtil.cpp +++ b/velox/connectors/hive/HiveConnectorUtil.cpp @@ -430,13 +430,29 @@ void configureReaderOptions( const Config* sessionProperties, const std::shared_ptr& hiveTableHandle, const std::shared_ptr& hiveSplit) { + configureReaderOptions( + readerOptions, + hiveConfig, + sessionProperties, + hiveTableHandle->dataColumns(), + hiveSplit, + hiveTableHandle->tableParameters()); +} + +void configureReaderOptions( + dwio::common::ReaderOptions& readerOptions, + const std::shared_ptr& hiveConfig, + const Config* sessionProperties, + const RowTypePtr& fileSchema, + const std::shared_ptr& hiveSplit, + const std::unordered_map& tableParameters) { readerOptions.setMaxCoalesceBytes(hiveConfig->maxCoalescedBytes()); readerOptions.setMaxCoalesceDistance(hiveConfig->maxCoalescedDistanceBytes()); readerOptions.setFileColumnNamesReadAsLowerCase( hiveConfig->isFileColumnNamesReadAsLowerCase(sessionProperties)); readerOptions.setUseColumnNamesForColumnMapping( hiveConfig->isOrcUseColumnNames(sessionProperties)); - readerOptions.setFileSchema(hiveTableHandle->dataColumns()); + readerOptions.setFileSchema(fileSchema); readerOptions.setFooterEstimatedSize(hiveConfig->footerEstimatedSize()); readerOptions.setFilePreloadThreshold(hiveConfig->filePreloadThreshold()); @@ -447,8 +463,8 @@ void configureReaderOptions( dwio::common::toString(readerOptions.getFileFormat()), dwio::common::toString(hiveSplit->fileFormat)); } else { - auto serDeOptions = parseSerdeParameters( - hiveSplit->serdeParameters, hiveTableHandle->tableParameters()); + auto serDeOptions = + parseSerdeParameters(hiveSplit->serdeParameters, tableParameters); if (serDeOptions) { readerOptions.setSerDeOptions(*serDeOptions); } diff --git a/velox/connectors/hive/HiveConnectorUtil.h b/velox/connectors/hive/HiveConnectorUtil.h index 67426bef78ca..329295b133d4 100644 --- a/velox/connectors/hive/HiveConnectorUtil.h +++ b/velox/connectors/hive/HiveConnectorUtil.h @@ -61,6 +61,14 @@ void configureReaderOptions( const std::shared_ptr& hiveTableHandle, const std::shared_ptr& hiveSplit); +void configureReaderOptions( + dwio::common::ReaderOptions& readerOptions, + const std::shared_ptr& hiveConfig, + const Config* sessionProperties, + const RowTypePtr& fileSchema, + const std::shared_ptr& hiveSplit, + const std::unordered_map& tableParameters = {}); + void configureRowReaderOptions( dwio::common::RowReaderOptions& rowReaderOptions, const std::unordered_map& tableParameters, diff --git a/velox/connectors/hive/SplitReader.cpp b/velox/connectors/hive/SplitReader.cpp index 92376e566d38..6395d5d5f8bb 100644 --- a/velox/connectors/hive/SplitReader.cpp +++ b/velox/connectors/hive/SplitReader.cpp @@ -21,6 +21,8 @@ #include "velox/connectors/hive/HiveConnectorSplit.h" #include "velox/connectors/hive/HiveConnectorUtil.h" #include "velox/connectors/hive/TableHandle.h" +#include "velox/connectors/hive/iceberg/IcebergSplitReader.h" +#include "velox/dwio/common/CachedBufferedInput.h" #include "velox/dwio/common/ReaderFactory.h" namespace facebook::velox::connector::hive { @@ -38,17 +40,33 @@ std::unique_ptr SplitReader::create( const ConnectorQueryCtx* connectorQueryCtx, const std::shared_ptr& hiveConfig, const std::shared_ptr& ioStats) { - return std::make_unique( - hiveSplit, - hiveTableHandle, - scanSpec, - readerOutputType, - partitionKeys, - fileHandleFactory, - executor, - connectorQueryCtx, - hiveConfig, - ioStats); + // Create the SplitReader based on hiveSplit->customSplitInfo["table_format"] + if (hiveSplit->customSplitInfo.count("table_format") > 0 && + hiveSplit->customSplitInfo["table_format"] == "hive-iceberg") { + return std::make_unique( + hiveSplit, + hiveTableHandle, + scanSpec, + readerOutputType, + partitionKeys, + fileHandleFactory, + executor, + connectorQueryCtx, + hiveConfig, + ioStats); + } else { + return std::make_unique( + hiveSplit, + hiveTableHandle, + scanSpec, + readerOutputType, + partitionKeys, + fileHandleFactory, + executor, + connectorQueryCtx, + hiveConfig, + ioStats); + } } SplitReader::SplitReader( diff --git a/velox/connectors/hive/iceberg/CMakeLists.txt b/velox/connectors/hive/iceberg/CMakeLists.txt new file mode 100644 index 000000000000..726ca63e31f3 --- /dev/null +++ b/velox/connectors/hive/iceberg/CMakeLists.txt @@ -0,0 +1,28 @@ +# Copyright (c) Facebook, Inc. and its affiliates. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +add_library( + velox_hive_iceberg_splitreader IcebergSplitReader.cpp IcebergSplit.cpp + PositionalDeleteFileReader.cpp) + +target_link_libraries( + velox_hive_iceberg_splitreader + Folly::folly + gflags::gflags + glog::glog + gtest + gtest_main + xsimd) + +add_subdirectory(tests) diff --git a/velox/connectors/hive/iceberg/IcebergDeleteFile.h b/velox/connectors/hive/iceberg/IcebergDeleteFile.h new file mode 100644 index 000000000000..2f9206dfc264 --- /dev/null +++ b/velox/connectors/hive/iceberg/IcebergDeleteFile.h @@ -0,0 +1,69 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include +#include +#include + +#include "velox/dwio/common/Options.h" + +namespace facebook::velox::connector::hive::iceberg { + +enum class FileContent { + kData, + kPositionalDeletes, + kEqualityDeletes, +}; + +struct IcebergDeleteFile { + FileContent content; + const std::string filePath; + dwio::common::FileFormat fileFormat; + uint64_t recordCount; + uint64_t fileSizeInBytes; + // The field ids for the delete columns for equality delete files + std::vector equalityFieldIds; + // The lower bounds of the in-file positions for the deleted rows, identified + // by each column's field id. E.g. The deleted rows for a column with field id + // 1 is in range [10, 50], where 10 and 50 are the deleted row positions in + // the data file, then lowerBounds would contain entry <1, "10"> + std::unordered_map lowerBounds; + // The upper bounds of the in-file positions for the deleted rows, identified + // by each column's field id. E.g. The deleted rows for a column with field id + // 1 is in range [10, 50], then upperBounds will contain entry <1, "50"> + std::unordered_map upperBounds; + + IcebergDeleteFile( + FileContent _content, + const std::string& _filePath, + dwio::common::FileFormat _fileFormat, + uint64_t _recordCount, + uint64_t _fileSizeInBytes, + std::vector _equalityFieldIds = {}, + std::unordered_map _lowerBounds = {}, + std::unordered_map _upperBounds = {}) + : content(_content), + filePath(_filePath), + fileFormat(_fileFormat), + recordCount(_recordCount), + fileSizeInBytes(_fileSizeInBytes), + equalityFieldIds(_equalityFieldIds), + lowerBounds(_lowerBounds), + upperBounds(_upperBounds) {} +}; + +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/IcebergMetadataColumns.h b/velox/connectors/hive/iceberg/IcebergMetadataColumns.h new file mode 100644 index 000000000000..4cbf2a7862b3 --- /dev/null +++ b/velox/connectors/hive/iceberg/IcebergMetadataColumns.h @@ -0,0 +1,55 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +#include "velox/type/Type.h" + +namespace facebook::velox::connector::hive::iceberg { + +struct IcebergMetadataColumn { + int id; + std::string name; + std::shared_ptr type; + std::string doc; + + IcebergMetadataColumn( + int _id, + const std::string& _name, + std::shared_ptr _type, + const std::string& _doc) + : id(_id), name(_name), type(_type), doc(_doc) {} + + static std::shared_ptr icebergDeleteFilePathColumn() { + return std::make_shared( + 2147483546, + "file_path", + VARCHAR(), + "Path of a file in which a deleted row is stored"); + } + + static std::shared_ptr icebergDeletePosColumn() { + return std::make_shared( + 2147483545, + "pos", + BIGINT(), + "Ordinal position of a deleted row in the data file"); + } +}; + +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/IcebergSplit.cpp b/velox/connectors/hive/iceberg/IcebergSplit.cpp new file mode 100644 index 000000000000..7fa9a52f2c69 --- /dev/null +++ b/velox/connectors/hive/iceberg/IcebergSplit.cpp @@ -0,0 +1,69 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "velox/connectors/hive/iceberg/IcebergSplit.h" + +#include "velox/connectors/hive/iceberg/IcebergDeleteFile.h" + +namespace facebook::velox::connector::hive::iceberg { + +HiveIcebergSplit::HiveIcebergSplit( + const std::string& _connectorId, + const std::string& _filePath, + dwio::common::FileFormat _fileFormat, + uint64_t _start, + uint64_t _length, + const std::unordered_map>& + _partitionKeys, + std::optional _tableBucketNumber, + const std::unordered_map& _customSplitInfo, + const std::shared_ptr& _extraFileInfo) + : HiveConnectorSplit( + _connectorId, + _filePath, + _fileFormat, + _start, + _length, + _partitionKeys, + _tableBucketNumber) { + // TODO: Deserialize _extraFileInfo to get deleteFiles; +} + +// For tests only +HiveIcebergSplit::HiveIcebergSplit( + const std::string& _connectorId, + const std::string& _filePath, + dwio::common::FileFormat _fileFormat, + uint64_t _start, + uint64_t _length, + const std::unordered_map>& + _partitionKeys, + std::optional _tableBucketNumber, + const std::unordered_map& _customSplitInfo, + const std::shared_ptr& _extraFileInfo, + std::vector _deletes) + : HiveConnectorSplit( + _connectorId, + _filePath, + _fileFormat, + _start, + _length, + _partitionKeys, + _tableBucketNumber, + _customSplitInfo, + _extraFileInfo), + deleteFiles(_deletes) {} +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/IcebergSplit.h b/velox/connectors/hive/iceberg/IcebergSplit.h new file mode 100644 index 000000000000..37b8c3c3eb36 --- /dev/null +++ b/velox/connectors/hive/iceberg/IcebergSplit.h @@ -0,0 +1,56 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include + +#include "velox/connectors/hive/HiveConnectorSplit.h" + +namespace facebook::velox::connector::hive::iceberg { + +class IcebergDeleteFile; + +struct HiveIcebergSplit : public connector::hive::HiveConnectorSplit { + std::vector deleteFiles; + + HiveIcebergSplit( + const std::string& connectorId, + const std::string& _filePath, + dwio::common::FileFormat _fileFormat, + uint64_t _start = 0, + uint64_t _length = std::numeric_limits::max(), + const std::unordered_map>& + _partitionKeys = {}, + std::optional _tableBucketNumber = std::nullopt, + const std::unordered_map& _customSplitInfo = {}, + const std::shared_ptr& _extraFileInfo = {}); + + // For tests only + HiveIcebergSplit( + const std::string& connectorId, + const std::string& _filePath, + dwio::common::FileFormat _fileFormat, + uint64_t _start = 0, + uint64_t _length = std::numeric_limits::max(), + const std::unordered_map>& + _partitionKeys = {}, + std::optional _tableBucketNumber = std::nullopt, + const std::unordered_map& _customSplitInfo = {}, + const std::shared_ptr& _extraFileInfo = {}, + std::vector deletes = {}); +}; + +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/IcebergSplitReader.cpp b/velox/connectors/hive/iceberg/IcebergSplitReader.cpp new file mode 100644 index 000000000000..fa65c41043e4 --- /dev/null +++ b/velox/connectors/hive/iceberg/IcebergSplitReader.cpp @@ -0,0 +1,113 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "velox/connectors/hive/iceberg/IcebergSplitReader.h" + +#include "velox/connectors/hive/iceberg/IcebergDeleteFile.h" +#include "velox/connectors/hive/iceberg/IcebergSplit.h" +#include "velox/dwio/common/BufferUtil.h" +#include "velox/dwio/common/Mutation.h" +#include "velox/dwio/common/Reader.h" + +using namespace facebook::velox::dwio::common; + +namespace facebook::velox::connector::hive::iceberg { + +IcebergSplitReader::IcebergSplitReader( + std::shared_ptr hiveSplit, + std::shared_ptr hiveTableHandle, + std::shared_ptr scanSpec, + const RowTypePtr readerOutputType, + std::unordered_map>* + partitionKeys, + FileHandleFactory* fileHandleFactory, + folly::Executor* executor, + const ConnectorQueryCtx* connectorQueryCtx, + const std::shared_ptr hiveConfig, + std::shared_ptr ioStats) + : SplitReader( + hiveSplit, + hiveTableHandle, + scanSpec, + readerOutputType, + partitionKeys, + fileHandleFactory, + executor, + connectorQueryCtx, + hiveConfig, + ioStats) {} + +void IcebergSplitReader::prepareSplit( + std::shared_ptr metadataFilter, + dwio::common::RuntimeStatistics& runtimeStats) { + SplitReader::prepareSplit(metadataFilter, runtimeStats); + baseReadOffset_ = 0; + positionalDeleteFileReaders_.clear(); + splitOffset_ = baseRowReader_->nextRowNumber(); + + // TODO: Deserialize the std::vector deleteFiles. For now + // we assume it's already deserialized. + std::shared_ptr icebergSplit = + std::dynamic_pointer_cast(hiveSplit_); + + const auto& deleteFiles = icebergSplit->deleteFiles; + for (const auto& deleteFile : deleteFiles) { + positionalDeleteFileReaders_.push_back( + std::make_unique( + deleteFile, + hiveSplit_->filePath, + fileHandleFactory_, + connectorQueryCtx_, + executor_, + hiveConfig_, + ioStats_, + runtimeStats, + splitOffset_, + hiveSplit_->connectorId)); + } +} + +uint64_t IcebergSplitReader::next(int64_t size, VectorPtr& output) { + Mutation mutation; + mutation.deletedRows = nullptr; + + if (!positionalDeleteFileReaders_.empty()) { + auto numBytes = bits::nbytes(size); + dwio::common::ensureCapacity( + deleteBitmap_, numBytes, connectorQueryCtx_->memoryPool()); + std::memset((void*)deleteBitmap_->as(), 0L, numBytes); + + for (auto iter = positionalDeleteFileReaders_.begin(); + iter != positionalDeleteFileReaders_.end(); + iter++) { + (*iter)->readDeletePositions( + baseReadOffset_, size, deleteBitmap_->asMutable()); + if ((*iter)->endOfFile()) { + iter = positionalDeleteFileReaders_.erase(iter); + } + } + + deleteBitmap_->setSize(numBytes); + mutation.deletedRows = deleteBitmap_->as(); + } + + auto rowsScanned = baseRowReader_->next(size, output, &mutation); + baseReadOffset_ += rowsScanned; + + return rowsScanned; +} + +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/IcebergSplitReader.h b/velox/connectors/hive/iceberg/IcebergSplitReader.h new file mode 100644 index 000000000000..5c5552369735 --- /dev/null +++ b/velox/connectors/hive/iceberg/IcebergSplitReader.h @@ -0,0 +1,62 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include "velox/connectors/Connector.h" +#include "velox/connectors/hive/SplitReader.h" +#include "velox/connectors/hive/iceberg/PositionalDeleteFileReader.h" + +namespace facebook::velox::connector::hive::iceberg { + +class IcebergDeleteFile; + +class IcebergSplitReader : public SplitReader { + public: + IcebergSplitReader( + std::shared_ptr hiveSplit, + std::shared_ptr hiveTableHandle, + std::shared_ptr scanSpec, + const RowTypePtr readerOutputType, + std::unordered_map>* + partitionKeys, + FileHandleFactory* fileHandleFactory, + folly::Executor* executor, + const ConnectorQueryCtx* connectorQueryCtx, + const std::shared_ptr hiveConfig, + std::shared_ptr ioStats); + + ~IcebergSplitReader() override = default; + + void prepareSplit( + std::shared_ptr metadataFilter, + dwio::common::RuntimeStatistics& runtimeStats) override; + + uint64_t next(int64_t size, VectorPtr& output) override; + + private: + // The read offset to the beginning of the split in number of rows for the + // current batch for the base data file + uint64_t baseReadOffset_; + + // The file position for the first row in the split + uint64_t splitOffset_; + + std::list> + positionalDeleteFileReaders_; + BufferPtr deleteBitmap_; +}; +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/PositionalDeleteFileReader.cpp b/velox/connectors/hive/iceberg/PositionalDeleteFileReader.cpp new file mode 100644 index 000000000000..b87007fb9804 --- /dev/null +++ b/velox/connectors/hive/iceberg/PositionalDeleteFileReader.cpp @@ -0,0 +1,243 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "velox/connectors/hive/iceberg/PositionalDeleteFileReader.h" + +#include "velox/connectors/hive/HiveConnectorUtil.h" +#include "velox/connectors/hive/TableHandle.h" +#include "velox/connectors/hive/iceberg/IcebergDeleteFile.h" +#include "velox/connectors/hive/iceberg/IcebergMetadataColumns.h" +#include "velox/dwio/common/ReaderFactory.h" + +namespace facebook::velox::connector::hive::iceberg { + +PositionalDeleteFileReader::PositionalDeleteFileReader( + const IcebergDeleteFile& deleteFile, + const std::string& baseFilePath, + FileHandleFactory* fileHandleFactory, + const ConnectorQueryCtx* connectorQueryCtx, + folly::Executor* executor, + const std::shared_ptr hiveConfig, + std::shared_ptr ioStats, + dwio::common::RuntimeStatistics& runtimeStats, + uint64_t splitOffset, + const std::string& connectorId) + : deleteFile_(deleteFile), + baseFilePath_(baseFilePath), + fileHandleFactory_(fileHandleFactory), + executor_(executor), + connectorQueryCtx_(connectorQueryCtx), + hiveConfig_(hiveConfig), + ioStats_(ioStats), + pool_(connectorQueryCtx->memoryPool()), + filePathColumn_(IcebergMetadataColumn::icebergDeleteFilePathColumn()), + posColumn_(IcebergMetadataColumn::icebergDeletePosColumn()), + splitOffset_(splitOffset), + deleteSplit_(nullptr), + deleteRowReader_(nullptr), + deletePositionsOutput_(nullptr), + deletePositionsOffset_(0), + endOfFile_(false) { + VELOX_CHECK(deleteFile_.content == FileContent::kPositionalDeletes); + + if (deleteFile_.recordCount == 0) { + return; + } + + // TODO: check if the lowerbounds and upperbounds in deleteFile overlap with + // this batch. If not, no need to proceed. + + // Create the ScanSpec for this delete file + auto scanSpec = std::make_shared(""); + scanSpec->addField(posColumn_->name, 0); + auto* pathSpec = + scanSpec->getOrCreateChild(common::Subfield(filePathColumn_->name)); + pathSpec->setFilter(std::make_unique( + std::vector({baseFilePath_}), false)); + + // Create the file schema (in RowType) and split that will be used by readers + std::vector deleteColumnNames( + {filePathColumn_->name, posColumn_->name}); + std::vector> deleteColumnTypes( + {filePathColumn_->type, posColumn_->type}); + RowTypePtr deleteFileSchema = + ROW(std::move(deleteColumnNames), std::move(deleteColumnTypes)); + + deleteSplit_ = std::make_shared( + connectorId, + deleteFile_.filePath, + deleteFile_.fileFormat, + 0, + deleteFile_.fileSizeInBytes); + + // Create the Reader and RowReader + + dwio::common::ReaderOptions deleteReaderOpts(pool_); + configureReaderOptions( + deleteReaderOpts, + hiveConfig_, + connectorQueryCtx_->sessionProperties(), + deleteFileSchema, + deleteSplit_); + + auto deleteFileHandle = + fileHandleFactory_->generate(deleteFile_.filePath).second; + auto deleteFileInput = createBufferedInput( + *deleteFileHandle, + deleteReaderOpts, + connectorQueryCtx_, + ioStats_, + executor_); + + auto deleteReader = + dwio::common::getReaderFactory(deleteReaderOpts.getFileFormat()) + ->createReader(std::move(deleteFileInput), deleteReaderOpts); + + // Check if the whole delete file split can be skipped. This could happen when + // 1) the delete file doesn't contain the base file that is being read; 2) The + // delete file does not contain the positions in the current batch for the + // base file. + if (!testFilters( + scanSpec.get(), + deleteReader.get(), + deleteSplit_->filePath, + deleteSplit_->partitionKeys, + {})) { + ++runtimeStats.skippedSplits; + runtimeStats.skippedSplitBytes += deleteSplit_->length; + deleteSplit_.reset(); + return; + } + + dwio::common::RowReaderOptions deleteRowReaderOpts; + configureRowReaderOptions( + deleteRowReaderOpts, + {}, + scanSpec, + nullptr, + deleteFileSchema, + deleteSplit_); + + deleteRowReader_.reset(); + deleteRowReader_ = deleteReader->createRowReader(deleteRowReaderOpts); +} + +void PositionalDeleteFileReader::readDeletePositions( + uint64_t baseReadOffset, + uint64_t size, + int8_t* deleteBitmap) { + // We are going to read to the row number up to the end of the batch. For the + // same base file, the deleted rows are in ascending order in the same delete + // file + int64_t rowNumberUpperBound = splitOffset_ + baseReadOffset + size; + + // Finish unused delete positions from last batch + if (deletePositionsOutput_ && + deletePositionsOffset_ < deletePositionsOutput_->size()) { + updateDeleteBitmap( + std::dynamic_pointer_cast(deletePositionsOutput_) + ->childAt(0), + baseReadOffset, + rowNumberUpperBound, + deleteBitmap); + + if (readFinishedForBatch(rowNumberUpperBound)) { + return; + } + } + + if (!deleteRowReader_ || !deleteSplit_) { + return; + } + + // Read the new delete positions for this batch into deletePositionsOutput_ + // and update the delete bitmap + + auto outputType = posColumn_->type; + + RowTypePtr outputRowType = ROW({posColumn_->name}, {posColumn_->type}); + if (!deletePositionsOutput_) { + deletePositionsOutput_ = BaseVector::create(outputRowType, 0, pool_); + } + + while (!readFinishedForBatch(rowNumberUpperBound)) { + auto rowsScanned = deleteRowReader_->next(size, deletePositionsOutput_); + if (rowsScanned > 0) { + VELOX_CHECK( + !deletePositionsOutput_->mayHaveNulls(), + "Iceberg delete file pos column cannot have nulls"); + + auto numDeletedRows = deletePositionsOutput_->size(); + if (numDeletedRows > 0) { + deletePositionsOutput_->loadedVector(); + deletePositionsOffset_ = 0; + + updateDeleteBitmap( + std::dynamic_pointer_cast(deletePositionsOutput_) + ->childAt(0), + baseReadOffset, + rowNumberUpperBound, + deleteBitmap); + } + } else { + // Reaching the end of the file + endOfFile_ = true; + deleteSplit_.reset(); + return; + } + } +} + +bool PositionalDeleteFileReader::endOfFile() { + return endOfFile_; +} + +void PositionalDeleteFileReader::updateDeleteBitmap( + VectorPtr deletePositionsVector, + uint64_t baseReadOffset, + int64_t rowNumberUpperBound, + int8_t* deleteBitmap) { + // Convert the positions in file into positions relative to the start of the + // split. + const int64_t* deletePositions = + deletePositionsVector->as>()->rawValues(); + int64_t offset = baseReadOffset + splitOffset_; + while (deletePositionsOffset_ < deletePositionsVector->size() && + deletePositions[deletePositionsOffset_] < rowNumberUpperBound) { + bits::setBit( + deleteBitmap, deletePositions[deletePositionsOffset_] - offset); + deletePositionsOffset_++; + } +} + +bool PositionalDeleteFileReader::readFinishedForBatch( + int64_t rowNumberUpperBound) { + VELOX_CHECK_NOT_NULL(deletePositionsOutput_); + + auto deletePositionsVector = + std::dynamic_pointer_cast(deletePositionsOutput_)->childAt(0); + const int64_t* deletePositions = + deletePositionsVector->as>()->rawValues(); + + if (deletePositionsOutput_->size() != 0 && + deletePositionsOffset_ < deletePositionsVector->size() && + deletePositions[deletePositionsOffset_] >= rowNumberUpperBound) { + return true; + } + return false; +} + +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/PositionalDeleteFileReader.h b/velox/connectors/hive/iceberg/PositionalDeleteFileReader.h new file mode 100644 index 000000000000..f6a1ddebcdb0 --- /dev/null +++ b/velox/connectors/hive/iceberg/PositionalDeleteFileReader.h @@ -0,0 +1,86 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +#include "velox/connectors/Connector.h" +#include "velox/connectors/hive/FileHandle.h" +#include "velox/connectors/hive/HiveConfig.h" +#include "velox/connectors/hive/HiveConnectorSplit.h" +#include "velox/dwio/common/Reader.h" + +namespace facebook::velox::connector::hive::iceberg { + +class IcebergDeleteFile; +class IcebergMetadataColumn; + +using SubfieldFilters = + std::unordered_map>; + +class PositionalDeleteFileReader { + public: + PositionalDeleteFileReader( + const IcebergDeleteFile& deleteFile, + const std::string& baseFilePath, + FileHandleFactory* fileHandleFactory, + const ConnectorQueryCtx* connectorQueryCtx, + folly::Executor* executor, + const std::shared_ptr hiveConfig, + std::shared_ptr ioStats, + dwio::common::RuntimeStatistics& runtimeStats, + uint64_t splitOffset, + const std::string& connectorId); + + void readDeletePositions( + uint64_t baseReadOffset, + uint64_t size, + int8_t* deleteBitmap); + + bool endOfFile(); + + private: + void updateDeleteBitmap( + VectorPtr deletePositionsVector, + uint64_t baseReadOffset, + int64_t rowNumberUpperBound, + int8_t* deleteBitmap); + + bool readFinishedForBatch(int64_t rowNumberUpperBound); + + const IcebergDeleteFile& deleteFile_; + const std::string& baseFilePath_; + FileHandleFactory* const fileHandleFactory_; + folly::Executor* const executor_; + const ConnectorQueryCtx* const connectorQueryCtx_; + const std::shared_ptr hiveConfig_; + std::shared_ptr ioStats_; + memory::MemoryPool* const pool_; + + std::shared_ptr filePathColumn_; + std::shared_ptr posColumn_; + uint64_t splitOffset_; + + std::shared_ptr deleteSplit_; + std::unique_ptr deleteRowReader_; + VectorPtr deletePositionsOutput_; + uint64_t deletePositionsOffset_; + bool endOfFile_; +}; + +} // namespace facebook::velox::connector::hive::iceberg diff --git a/velox/connectors/hive/iceberg/tests/CMakeLists.txt b/velox/connectors/hive/iceberg/tests/CMakeLists.txt new file mode 100644 index 000000000000..63603c724ec2 --- /dev/null +++ b/velox/connectors/hive/iceberg/tests/CMakeLists.txt @@ -0,0 +1,34 @@ +# Copyright (c) Facebook, Inc. and its affiliates. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +if(NOT VELOX_DISABLE_GOOGLETEST) + + add_executable(velox_hive_iceberg_test IcebergReadTest.cpp) + add_test(velox_hive_iceberg_test velox_hive_iceberg_test) + + target_link_libraries( + velox_hive_iceberg_test + velox_hive_connector + velox_hive_iceberg_splitreader + velox_hive_partition_function + velox_dwio_common_exception + velox_dwio_common_test_utils + velox_dwio_dwrf_proto + velox_vector_test_lib + velox_exec + velox_exec_test_lib + Folly::folly + gtest + gtest_main) + +endif() diff --git a/velox/connectors/hive/iceberg/tests/IcebergReadTest.cpp b/velox/connectors/hive/iceberg/tests/IcebergReadTest.cpp new file mode 100644 index 000000000000..79443c73b3ce --- /dev/null +++ b/velox/connectors/hive/iceberg/tests/IcebergReadTest.cpp @@ -0,0 +1,280 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "velox/common/file/FileSystems.h" +#include "velox/connectors/hive/HiveConnectorSplit.h" +#include "velox/connectors/hive/iceberg/IcebergDeleteFile.h" +#include "velox/connectors/hive/iceberg/IcebergMetadataColumns.h" +#include "velox/connectors/hive/iceberg/IcebergSplit.h" +#include "velox/exec/PlanNodeStats.h" +#include "velox/exec/tests/utils/HiveConnectorTestBase.h" +#include "velox/exec/tests/utils/PlanBuilder.h" + +#include + +using namespace facebook::velox::exec::test; +using namespace facebook::velox::exec; +using namespace facebook::velox::dwio::common; +using namespace facebook::velox::test; + +namespace facebook::velox::connector::hive::iceberg { + +class HiveIcebergTest : public HiveConnectorTestBase { + public: + void assertPositionalDeletes( + const std::vector& deleteRows, + bool multipleBaseFiles = false) { + assertPositionalDeletes( + deleteRows, + "SELECT * FROM tmp WHERE c0 NOT IN (" + makeNotInList(deleteRows) + ")", + multipleBaseFiles); + } + void assertPositionalDeletes( + const std::vector& deleteRows, + std::string duckdbSql, + bool multipleBaseFiles = false) { + std::shared_ptr dataFilePath = writeDataFile(rowCount); + + std::mt19937 gen{0}; + int64_t numDeleteRowsBefore = + multipleBaseFiles ? folly::Random::rand32(0, 1000, gen) : 0; + int64_t numDeleteRowsAfter = + multipleBaseFiles ? folly::Random::rand32(0, 1000, gen) : 0; + std::shared_ptr deleteFilePath = writePositionDeleteFile( + dataFilePath->path, + deleteRows, + numDeleteRowsBefore, + numDeleteRowsAfter); + + IcebergDeleteFile deleteFile( + FileContent::kPositionalDeletes, + deleteFilePath->path, + fileFomat_, + deleteRows.size() + numDeleteRowsBefore + numDeleteRowsAfter, + testing::internal::GetFileSize( + std::fopen(deleteFilePath->path.c_str(), "r"))); + + auto icebergSplit = makeIcebergSplit(dataFilePath->path, {deleteFile}); + + auto plan = tableScanNode(); + auto task = OperatorTestBase::assertQuery(plan, {icebergSplit}, duckdbSql); + + auto planStats = toPlanStats(task->taskStats()); + auto scanNodeId = plan->id(); + auto it = planStats.find(scanNodeId); + ASSERT_TRUE(it != planStats.end()); + ASSERT_TRUE(it->second.peakMemoryBytes > 0); + } + + std::vector makeRandomDeleteRows(int32_t maxRowNumber) { + std::mt19937 gen{0}; + std::vector deleteRows; + for (int i = 0; i < maxRowNumber; i++) { + if (folly::Random::rand32(0, 10, gen) > 8) { + deleteRows.push_back(i); + } + } + return deleteRows; + } + + std::vector makeSequenceRows(int32_t maxRowNumber) { + std::vector deleteRows; + deleteRows.resize(maxRowNumber); + std::iota(deleteRows.begin(), deleteRows.end(), 0); + return deleteRows; + } + + const static int rowCount = 20000; + + private: + std::shared_ptr makeIcebergSplit( + const std::string& dataFilePath, + const std::vector& deleteFiles = {}) { + std::unordered_map> partitionKeys; + std::unordered_map customSplitInfo; + customSplitInfo["table_format"] = "hive-iceberg"; + + auto file = filesystems::getFileSystem(dataFilePath, nullptr) + ->openFileForRead(dataFilePath); + const int64_t fileSize = file->size(); + + return std::make_shared( + kHiveConnectorId, + dataFilePath, + fileFomat_, + 0, + fileSize, + partitionKeys, + std::nullopt, + customSplitInfo, + nullptr, + deleteFiles); + } + + std::vector makeVectors(int32_t count, int32_t rowsPerVector) { + std::vector vectors; + + for (int i = 0; i < count; i++) { + auto data = makeSequenceRows(rowsPerVector); + VectorPtr c0 = vectorMaker_.flatVector(data); + vectors.push_back(makeRowVector({"c0"}, {c0})); + } + + return vectors; + } + + std::shared_ptr writeDataFile(uint64_t numRows) { + auto dataVectors = makeVectors(1, numRows); + + auto dataFilePath = TempFilePath::create(); + writeToFile(dataFilePath->path, dataVectors); + createDuckDbTable(dataVectors); + return dataFilePath; + } + + std::shared_ptr writePositionDeleteFile( + const std::string& dataFilePath, + const std::vector& deleteRows, + int64_t numRowsBefore = 0, + int64_t numRowsAfter = 0) { + // if containsMultipleDataFiles == true, we will write rows for other base + // files before and after the target base file + uint32_t numDeleteRows = numRowsBefore + deleteRows.size() + numRowsAfter; + + std::string dataFilePathBefore = dataFilePath + "_before"; + std::string dataFilePathAfter = dataFilePath + "_after"; + + auto filePathVector = + vectorMaker_.flatVector(numDeleteRows, [&](auto row) { + if (row < numRowsBefore) { + return StringView(dataFilePathBefore); + } else if ( + row >= numRowsBefore && row < deleteRows.size() + numRowsBefore) { + return StringView(dataFilePath); + } else if ( + row >= deleteRows.size() + numRowsBefore && row < numDeleteRows) { + return StringView(dataFilePathAfter); + } else { + return StringView(); + } + }); + + std::vector deleteRowsVec; + deleteRowsVec.reserve(numDeleteRows); + + if (numRowsBefore > 0) { + auto rowsBefore = makeSequenceRows(numRowsBefore); + deleteRowsVec.insert( + deleteRowsVec.end(), rowsBefore.begin(), rowsBefore.end()); + } + deleteRowsVec.insert( + deleteRowsVec.end(), deleteRows.begin(), deleteRows.end()); + if (numRowsAfter > 0) { + auto rowsAfter = makeSequenceRows(numRowsAfter); + deleteRowsVec.insert( + deleteRowsVec.end(), rowsAfter.begin(), rowsAfter.end()); + } + + auto deletePositionsVector = + vectorMaker_.flatVector(deleteRowsVec); + RowVectorPtr deleteFileVectors = makeRowVector( + {pathColumn_->name, posColumn_->name}, + {filePathVector, deletePositionsVector}); + + auto deleteFilePath = TempFilePath::create(); + writeToFile(deleteFilePath->path, deleteFileVectors); + + return deleteFilePath; + } + + std::string makeNotInList(const std::vector& deleteRows) { + if (deleteRows.empty()) { + return ""; + } + + return std::accumulate( + deleteRows.begin() + 1, + deleteRows.end(), + std::to_string(deleteRows[0]), + [](const std::string& a, int64_t b) { + return a + ", " + std::to_string(b); + }); + } + + std::shared_ptr assertQuery( + const core::PlanNodePtr& plan, + std::shared_ptr dataFilePath, + const std::vector& deleteFiles, + const std::string& duckDbSql) { + auto icebergSplit = makeIcebergSplit(dataFilePath->path, deleteFiles); + return OperatorTestBase::assertQuery(plan, {icebergSplit}, duckDbSql); + } + + core::PlanNodePtr tableScanNode() { + return PlanBuilder(pool_.get()).tableScan(rowType_).planNode(); + } + + private: + dwio::common::FileFormat fileFomat_{dwio::common::FileFormat::DWRF}; + RowTypePtr rowType_{ROW({"c0"}, {BIGINT()})}; + std::shared_ptr pathColumn_ = + IcebergMetadataColumn::icebergDeleteFilePathColumn(); + std::shared_ptr posColumn_ = + IcebergMetadataColumn::icebergDeletePosColumn(); +}; + +TEST_F(HiveIcebergTest, positionalDeletesSingleBaseFile) { + folly::SingletonVault::singleton()->registrationComplete(); + + // Delete row 0, 1, 2, 3 from the first batch out of two. + assertPositionalDeletes({0, 1, 2, 3}); + // Delete the first and last row in each batch (10000 rows per batch) + assertPositionalDeletes({0, 9999, 10000, 19999}); + // Delete several rows in the second batch (10000 rows per batch) + assertPositionalDeletes({10000, 10002, 19999}); + // Delete random rows + assertPositionalDeletes(makeRandomDeleteRows(rowCount)); + // Delete 0 rows + assertPositionalDeletes({}, "SELECT * FROM tmp", false); + // Delete all rows + assertPositionalDeletes( + makeSequenceRows(rowCount), "SELECT * FROM tmp WHERE 1 = 0", false); + // Delete rows that don't exist + assertPositionalDeletes({20000, 29999}); +} + +// The positional delete file contains rows from multiple base files +TEST_F(HiveIcebergTest, positionalDeletesMultipleBaseFiles) { + folly::SingletonVault::singleton()->registrationComplete(); + + // Delete row 0, 1, 2, 3 from the first batch out of two. + assertPositionalDeletes({0, 1, 2, 3}, true); + // Delete the first and last row in each batch (10000 rows per batch) + assertPositionalDeletes({0, 9999, 10000, 19999}, true); + // Delete several rows in the second batch (10000 rows per batch) + assertPositionalDeletes({10000, 10002, 19999}, true); + // Delete random rows + assertPositionalDeletes(makeRandomDeleteRows(rowCount), true); + // Delete 0 rows + assertPositionalDeletes({}, "SELECT * FROM tmp", true); + // Delete all rows + assertPositionalDeletes( + makeSequenceRows(rowCount), "SELECT * FROM tmp WHERE 1 = 0", true); + // Delete rows that don't exist + assertPositionalDeletes({20000, 29999}, true); +} + +} // namespace facebook::velox::connector::hive::iceberg From 021b2298e0cf8dd3f9820d90475b7a4be589db97 Mon Sep 17 00:00:00 2001 From: zhli1142015 Date: Wed, 14 Feb 2024 07:53:05 -0800 Subject: [PATCH 19/38] Remove duplicate line in arrayGroupProbe (#8745) Summary: Line 584 should be duplicate with line 583, we should not set the result twice. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8745 Reviewed By: Yuhta Differential Revision: D53758874 Pulled By: mbasmanova fbshipit-source-id: 5bb6a74aa948fab47253a69f1dc29efdc87aed9c --- velox/exec/HashTable.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/velox/exec/HashTable.cpp b/velox/exec/HashTable.cpp index d09b3bc041f4..44cd1e82e273 100644 --- a/velox/exec/HashTable.cpp +++ b/velox/exec/HashTable.cpp @@ -580,8 +580,7 @@ void HashTable::arrayGroupProbe(HashLookup& lookup) { if (UNLIKELY(!group)) { group = insertEntry(lookup, index, row); } - groups[row] = group; - lookup.hits[row] = group; // NOLINT + groups[row] = group; // NOLINT } } From 2eaf155c522e6bc2a6b498ed0d86482a5ff11847 Mon Sep 17 00:00:00 2001 From: Kevin Wilfong Date: Wed, 14 Feb 2024 10:25:55 -0800 Subject: [PATCH 20/38] Remove the notion of encodings from PrestoIterativeVectorSerializer (#8606) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/8606 PrestoBatchVectorSerializer supports maintaining encodings while serializing through the BatchVectorSerializer interface. We no longer need it in PrestoIterativeVectorSerializer which is a little dangerous (e.g. append can be called multiple times before flush breaking serialization). Reviewed By: bikramSingh91 Differential Revision: D53202362 fbshipit-source-id: 9a67f972af8db37fb114b69eef9771254744211c --- velox/serializers/PrestoSerializer.cpp | 56 ------------------- velox/serializers/PrestoSerializer.h | 22 -------- .../tests/PrestoSerializerTest.cpp | 44 --------------- 3 files changed, 122 deletions(-) diff --git a/velox/serializers/PrestoSerializer.cpp b/velox/serializers/PrestoSerializer.cpp index 149a70948fc5..bc9843174699 100644 --- a/velox/serializers/PrestoSerializer.cpp +++ b/velox/serializers/PrestoSerializer.cpp @@ -3320,7 +3320,6 @@ class PrestoIterativeVectorSerializer : public IterativeVectorSerializer { public: PrestoIterativeVectorSerializer( const RowTypePtr& rowType, - std::vector encodings, int32_t numRows, StreamArena* streamArena, bool useLosslessTimestamp, @@ -3332,40 +3331,10 @@ class PrestoIterativeVectorSerializer : public IterativeVectorSerializer { streams_.resize(numTypes); for (int i = 0; i < numTypes; ++i) { - std::optional encoding = std::nullopt; - if (i < encodings.size()) { - encoding = encodings[i]; - } streams_[i] = std::make_unique( types[i], - encoding, std::nullopt, - streamArena, - numRows, - useLosslessTimestamp); - } - } - - // Constructor that takes a row vector instead of only the types. This is - // different because then we know exactly how each vector is encoded - // (recursively). - PrestoIterativeVectorSerializer( - const RowVectorPtr& rowVector, - StreamArena* streamArena, - bool useLosslessTimestamp, - common::CompressionKind compressionKind) - : streamArena_(streamArena), - codec_(common::compressionKindToCodec(compressionKind)) { - auto numRows = rowVector->size(); - auto rowType = rowVector->type(); - auto numChildren = rowVector->childrenSize(); - streams_.resize(numChildren); - - for (int i = 0; i < numChildren; i++) { - streams_[i] = std::make_unique( - rowType->childAt(i), std::nullopt, - rowVector->childAt(i), streamArena, numRows, useLosslessTimestamp); @@ -3420,16 +3389,6 @@ class PrestoIterativeVectorSerializer : public IterativeVectorSerializer { flushStreams(streams_, numRows_, *streamArena_, *codec_, out); } - void flushEncoded(const RowVectorPtr& vector, OutputStream* out) { - VELOX_CHECK_EQ(0, numRows_); - - std::vector ranges{{0, vector->size()}}; - Scratch scratch; - append(vector, folly::Range(ranges.data(), ranges.size()), scratch); - - flushStreams(streams_, vector->size(), *streamArena_, *codec_, out); - } - private: StreamArena* const streamArena_; const std::unique_ptr codec_; @@ -3464,7 +3423,6 @@ PrestoVectorSerde::createIterativeSerializer( const auto prestoOptions = toPrestoOptions(options); return std::make_unique( type, - prestoOptions.encodings, numRows, streamArena, prestoOptions.useLosslessTimestamp, @@ -3479,20 +3437,6 @@ std::unique_ptr PrestoVectorSerde::createBatchSerializer( pool, prestoOptions.useLosslessTimestamp, prestoOptions.compressionKind); } -void PrestoVectorSerde::deprecatedSerializeEncoded( - const RowVectorPtr& vector, - StreamArena* streamArena, - const Options* options, - OutputStream* out) { - auto prestoOptions = toPrestoOptions(options); - auto serializer = std::make_unique( - vector, - streamArena, - prestoOptions.useLosslessTimestamp, - prestoOptions.compressionKind); - serializer->flushEncoded(vector, out); -} - void PrestoVectorSerde::deserialize( ByteInputStream* source, velox::memory::MemoryPool* pool, diff --git a/velox/serializers/PrestoSerializer.h b/velox/serializers/PrestoSerializer.h index a957e27ce76e..287741afe0c8 100644 --- a/velox/serializers/PrestoSerializer.h +++ b/velox/serializers/PrestoSerializer.h @@ -58,9 +58,6 @@ class PrestoVectorSerde : public VectorSerde { bool useLosslessTimestamp{false}; common::CompressionKind compressionKind{ common::CompressionKind::CompressionKind_NONE}; - - /// Specifies the encoding for each of the top-level child vector. - std::vector encodings; }; /// Adds the serialized sizes of the rows of 'vector' in 'ranges[i]' to @@ -90,25 +87,6 @@ class PrestoVectorSerde : public VectorSerde { memory::MemoryPool* pool, const Options* options) override; - /// Serializes a single RowVector with possibly encoded children, preserving - /// their encodings. Encodings are preserved recursively for any RowVector - /// children, but not for children of other nested vectors such as Array, Map, - /// and Dictionary. - /// - /// PrestoPage does not support serialization of Dictionaries with nulls; - /// in case dictionaries contain null they are serialized as flat buffers. - /// - /// In order to override the encodings of top-level columns in the RowVector, - /// you can specifiy the encodings using PrestoOptions.encodings - /// - /// DEPRECATED: Use createBatchSerializer and the BatchVectorSerializer's - /// serialize function instead. - void deprecatedSerializeEncoded( - const RowVectorPtr& vector, - StreamArena* streamArena, - const Options* options, - OutputStream* out); - bool supportsAppendInDeserialize() const override { return true; } diff --git a/velox/serializers/tests/PrestoSerializerTest.cpp b/velox/serializers/tests/PrestoSerializerTest.cpp index 6c75a67d6c3a..143abc7dc2cb 100644 --- a/velox/serializers/tests/PrestoSerializerTest.cpp +++ b/velox/serializers/tests/PrestoSerializerTest.cpp @@ -253,23 +253,6 @@ class PrestoSerializerTest return stats; } - void serializeEncoded( - const RowVectorPtr& rowVector, - std::ostream* output, - const serializer::presto::PrestoVectorSerde::PrestoOptions* - serdeOptions) { - facebook::velox::serializer::presto::PrestoOutputStreamListener listener; - OStreamOutputStream out(output, &listener); - StreamArena arena{pool_.get()}; - auto paramOptions = getParamSerdeOptions(serdeOptions); - - for (const auto& child : rowVector->children()) { - paramOptions.encodings.push_back(child->encoding()); - } - - serde_->deprecatedSerializeEncoded(rowVector, &arena, ¶mOptions, &out); - } - void assertEqualEncoding( const RowVectorPtr& expected, const RowVectorPtr& actual) { @@ -316,17 +299,6 @@ class PrestoSerializerTest assertEqualVectors(expected, result); } - void testEncodedRoundTrip( - const RowVectorPtr& data, - const serializer::presto::PrestoVectorSerde::PrestoOptions* serdeOptions = - nullptr) { - std::ostringstream out; - serializeEncoded(data, &out, serdeOptions); - const auto serialized = out.str(); - - verifySerializedEncodedData(data, serialized, serdeOptions); - } - void serializeBatch( const RowVectorPtr& rowVector, std::ostream* output, @@ -744,34 +716,18 @@ TEST_P(PrestoSerializerTest, longDecimal) { testRoundTrip(vector); } -// Test that hierarchically encoded columns (rows) have their encodings -// preserved. -TEST_P(PrestoSerializerTest, encodings) { - testEncodedRoundTrip(encodingsTestVector()); -} - // Test that hierarchically encoded columns (rows) have their encodings // preserved by the PrestoBatchVectorSerializer. TEST_P(PrestoSerializerTest, encodingsBatchVectorSerializer) { testBatchVectorSerializerRoundTrip(encodingsTestVector()); } -// Test that array elements have their encodings preserved. -TEST_P(PrestoSerializerTest, encodingsArrayElements) { - testEncodedRoundTrip(encodingsArrayElementsTestVector()); -} - // Test that array elements have their encodings preserved by the // PrestoBatchVectorSerializer. TEST_P(PrestoSerializerTest, encodingsArrayElementsBatchVectorSerializer) { testBatchVectorSerializerRoundTrip(encodingsArrayElementsTestVector()); } -// Test that map values have their encodings preserved. -TEST_P(PrestoSerializerTest, encodingsMapValues) { - testEncodedRoundTrip(encodingsMapValuesTestVector()); -} - // Test that map values have their encodings preserved by the // PrestoBatchVectorSerializer. TEST_P(PrestoSerializerTest, encodingsMapValuesBatchVectorSerializer) { From 159e7e972393eb0d7c974e10c8d85652967bb06b Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Wed, 14 Feb 2024 10:31:41 -0800 Subject: [PATCH 21/38] Improve Centos8 setup script (#8683) Summary: Improve centos8 setup script to match the MacOS and Ubuntu setup scripts. Move common functions to setup-helper-functions.sh Pull Request resolved: https://github.com/facebookincubator/velox/pull/8683 Reviewed By: mbasmanova Differential Revision: D53600088 Pulled By: kgpai fbshipit-source-id: c508134a253b012e63a7504eb6acdd914d9c32aa --- scripts/setup-centos8.sh | 210 ++++++++++++++++++++---------- scripts/setup-helper-functions.sh | 22 +++- scripts/setup-macos.sh | 20 --- scripts/setup-ubuntu.sh | 22 +--- 4 files changed, 162 insertions(+), 112 deletions(-) diff --git a/scripts/setup-centos8.sh b/scripts/setup-centos8.sh index 60a20d49c7e9..2df599e0d0d8 100755 --- a/scripts/setup-centos8.sh +++ b/scripts/setup-centos8.sh @@ -30,19 +30,16 @@ function dnf_install { dnf install -y -q --setopt=install_weak_deps=False "$@" } +dnf update -y dnf_install epel-release dnf-plugins-core # For ccache, ninja dnf config-manager --set-enabled powertools +dnf update -y dnf_install ninja-build cmake curl ccache gcc-toolset-9 git wget which libevent-devel \ openssl-devel re2-devel libzstd-devel lz4-devel double-conversion-devel \ libdwarf-devel curl-devel libicu-devel -dnf remove -y gflags - -# Required for Thrift dnf_install autoconf automake libtool bison flex python3 libsodium-devel -dnf_install conda - # install sphinx for doc gen pip3 install sphinx sphinx-tabs breathe sphinx_rtd_theme @@ -50,83 +47,156 @@ pip3 install sphinx sphinx-tabs breathe sphinx_rtd_theme source /opt/rh/gcc-toolset-9/enable || exit 1 set -u -function cmake_install { - cmake -B "$1-build" -GNinja -DCMAKE_CXX_STANDARD=17 \ - -DCMAKE_CXX_FLAGS="${CFLAGS}" -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_BUILD_TYPE="${CMAKE_BUILD_TYPE}" -Wno-dev "$@" - ninja -C "$1-build" install +function install_conda { + dnf_install conda } -# Fetch sources. -wget_and_untar https://github.com/gflags/gflags/archive/v2.2.2.tar.gz gflags -wget_and_untar https://github.com/google/glog/archive/v0.6.0.tar.gz glog -wget_and_untar http://www.oberhumer.com/opensource/lzo/download/lzo-2.10.tar.gz lzo -wget_and_untar https://boostorg.jfrog.io/artifactory/main/release/1.72.0/source/boost_1_72_0.tar.gz boost -wget_and_untar https://github.com/google/snappy/archive/1.1.8.tar.gz snappy -wget_and_untar https://github.com/fmtlib/fmt/archive/10.1.1.tar.gz fmt +function install_gflags { + # Remove an older version if present. + dnf remove -y gflags + wget_and_untar https://github.com/gflags/gflags/archive/v2.2.2.tar.gz gflags + ( + cd gflags + cmake_install -DBUILD_SHARED_LIBS=ON -DBUILD_STATIC_LIBS=ON -DBUILD_gflags_LIB=ON -DLIB_SUFFIX=64 + ) +} -wget_and_untar https://github.com/protocolbuffers/protobuf/releases/download/v21.4/protobuf-all-21.4.tar.gz protobuf +function install_glog { + wget_and_untar https://github.com/google/glog/archive/v0.6.0.tar.gz glog + ( + cd glog + cmake_install -DBUILD_SHARED_LIBS=ON + ) +} + +function install_lzo { + wget_and_untar http://www.oberhumer.com/opensource/lzo/download/lzo-2.10.tar.gz lzo + ( + cd lzo + ./configure --prefix=/usr --enable-shared --disable-static --docdir=/usr/share/doc/lzo-2.10 + make "-j$(nproc)" + make install + ) +} + +function install_boost { + wget_and_untar https://boostorg.jfrog.io/artifactory/main/release/1.72.0/source/boost_1_72_0.tar.gz boost + ( + cd boost + ./bootstrap.sh --prefix=/usr/local + ./b2 "-j$(nproc)" -d0 install threading=multi + ) +} + +function install_snappy { + wget_and_untar https://github.com/google/snappy/archive/1.1.8.tar.gz snappy + ( + cd snappy + cmake_install -DSNAPPY_BUILD_TESTS=OFF + ) +} + +function install_fmt { + wget_and_untar https://github.com/fmtlib/fmt/archive/10.1.1.tar.gz fmt + ( + cd fmt + cmake_install -DFMT_TEST=OFF + ) +} + +function install_protobuf { + wget_and_untar https://github.com/protocolbuffers/protobuf/releases/download/v21.4/protobuf-all-21.4.tar.gz protobuf + ( + cd protobuf + ./configure --prefix=/usr + make "-j${NPROC}" + make install + ldconfig + ) +} FB_OS_VERSION="v2023.12.04.00" -wget_and_untar https://github.com/facebookincubator/fizz/archive/refs/tags/${FB_OS_VERSION}.tar.gz fizz -wget_and_untar https://github.com/facebook/folly/archive/refs/tags/${FB_OS_VERSION}.tar.gz folly -wget_and_untar https://github.com/facebook/wangle/archive/refs/tags/${FB_OS_VERSION}.tar.gz wangle -wget_and_untar https://github.com/facebook/fbthrift/archive/refs/tags/${FB_OS_VERSION}.tar.gz fbthrift -wget_and_untar https://github.com/facebook/mvfst/archive/refs/tags/${FB_OS_VERSION}.tar.gz mvfst +function install_fizz { + wget_and_untar https://github.com/facebookincubator/fizz/archive/refs/tags/${FB_OS_VERSION}.tar.gz fizz + ( + cd fizz/fizz + cmake_install -DBUILD_TESTS=OFF + ) +} -wait # For cmake and source downloads to complete. +function install_folly { + wget_and_untar https://github.com/facebook/folly/archive/refs/tags/${FB_OS_VERSION}.tar.gz folly + ( + cd folly + cmake_install -DFOLLY_HAVE_INT128_T=ON + ) +} -# Build & install. -( - cd lzo - ./configure --prefix=/usr --enable-shared --disable-static --docdir=/usr/share/doc/lzo-2.10 - make "-j$(nproc)" - make install -) +function install_wangle { + wget_and_untar https://github.com/facebook/wangle/archive/refs/tags/${FB_OS_VERSION}.tar.gz wangle + ( + cd wangle/wangle + cmake_install -DBUILD_TESTS=OFF + ) +} -( - cd boost - ./bootstrap.sh --prefix=/usr/local - ./b2 "-j$(nproc)" -d0 install threading=multi -) +function install_fbthrift { + wget_and_untar https://github.com/facebook/fbthrift/archive/refs/tags/${FB_OS_VERSION}.tar.gz fbthrift + ( + cd fbthrift + cmake_install -Denable_tests=OFF + ) +} + +function install_mvfst { + wget_and_untar https://github.com/facebook/mvfst/archive/refs/tags/${FB_OS_VERSION}.tar.gz mvfst + ( + cd mvfst + cmake_install -DBUILD_TESTS=OFF + ) +} + +function install_duckdb { + if $BUILD_DUCKDB ; then + echo 'Building DuckDB' + wget_and_untar https://github.com/duckdb/duckdb/archive/refs/tags/v0.8.1.tar.gz duckdb + ( + cd duckdb + cmake_install -DBUILD_UNITTESTS=OFF -DENABLE_SANITIZER=OFF -DENABLE_UBSAN=OFF -DBUILD_SHELL=OFF -DEXPORT_DLL_SYMBOLS=OFF -DCMAKE_BUILD_TYPE=Release + ) + fi +} + +function install_velox_deps { + run_and_time install_conda + run_and_time install_gflags + run_and_time install_glog + run_and_time install_lzo + run_and_time install_snappy + run_and_time install_boost + run_and_time install_protobuf + run_and_time install_fmt + run_and_time install_folly + run_and_time install_fizz + run_and_time install_wangle + run_and_time install_mvfst + run_and_time install_fbthrift + run_and_time install_duckdb +} + +(return 2> /dev/null) && return # If script was sourced, don't run commands. ( - cd protobuf - ./configure --prefix=/usr - make "-j${NPROC}" - make install - ldconfig + if [[ $# -ne 0 ]]; then + for cmd in "$@"; do + run_and_time "${cmd}" + done + else + install_velox_deps + fi ) -cmake_install gflags -DBUILD_SHARED_LIBS=ON -DBUILD_STATIC_LIBS=ON -DBUILD_gflags_LIB=ON -DLIB_SUFFIX=64 -DCMAKE_INSTALL_PREFIX:PATH=/usr -cmake_install glog -DBUILD_SHARED_LIBS=ON -DCMAKE_INSTALL_PREFIX:PATH=/usr -cmake_install snappy -DSNAPPY_BUILD_TESTS=OFF -cmake_install fmt -DFMT_TEST=OFF -cmake_install folly -DFOLLY_HAVE_INT128_T=ON - -cmake_install fizz/fizz -DBUILD_TESTS=OFF -cmake_install wangle/wangle -DBUILD_TESTS=OFF -cmake_install mvfst -DBUILD_TESTS=OFF -cmake_install fbthrift -Denable_tests=OFF - -if $BUILD_DUCKDB ; then - echo 'Building DuckDB' - mkdir ~/duckdb-install && cd ~/duckdb-install - wget https://github.com/duckdb/duckdb/archive/refs/tags/v0.8.1.tar.gz - tar -xf v0.8.1.tar.gz - cd duckdb-0.8.1 - mkdir build && cd build - CMAKE_FLAGS=( - "-DBUILD_UNITTESTS=OFF" - "-DENABLE_SANITIZER=OFF" - "-DENABLE_UBSAN=OFF" - "-DBUILD_SHELL=OFF" - "-DEXPORT_DLL_SYMBOLS=OFF" - "-DCMAKE_BUILD_TYPE=Release" - ) - cmake ${CMAKE_FLAGS[*]} .. - make install -j 16 - rm -rf ~/duckdb-install -fi +echo "All dependencies for Velox installed!" dnf clean all diff --git a/scripts/setup-helper-functions.sh b/scripts/setup-helper-functions.sh index 9495d42bfc36..4f0a11e152fd 100644 --- a/scripts/setup-helper-functions.sh +++ b/scripts/setup-helper-functions.sh @@ -15,6 +15,27 @@ # github_checkout $REPO $VERSION $GIT_CLONE_PARAMS clones or re-uses an existing clone of the # specified repo, checking out the requested version. + +function run_and_time { + time "$@" || (echo "Failed to run $* ." ; exit 1 ) + { echo "+ Finished running $*"; } 2> /dev/null +} + +function prompt { + ( + while true; do + local input="${PROMPT_ALWAYS_RESPOND:-}" + echo -n "$(tput bold)$* [Y, n]$(tput sgr0) " + [[ -z "${input}" ]] && read input + if [[ "${input}" == "Y" || "${input}" == "y" || "${input}" == "" ]]; then + return 0 + elif [[ "${input}" == "N" || "${input}" == "n" ]]; then + return 1 + fi + done + ) 2> /dev/null +} + function github_checkout { local REPO=$1 shift @@ -36,7 +57,6 @@ function github_checkout { cd "${DIRNAME}" } - # get_cxx_flags [$CPU_ARCH] # Sets and exports the variable VELOX_CXX_FLAGS with appropriate compiler flags. # If $CPU_ARCH is set then we use that else we determine best possible set of flags diff --git a/scripts/setup-macos.sh b/scripts/setup-macos.sh index 197ea54e8394..7872195b9e77 100755 --- a/scripts/setup-macos.sh +++ b/scripts/setup-macos.sh @@ -38,26 +38,6 @@ MACOS_DEPS="ninja flex bison cmake ccache protobuf@21 icu4c boost gflags glog li FB_OS_VERSION="v2023.12.04.00" -function run_and_time { - time "$@" || (echo "Failed to run $* ." ; exit 1 ) - { echo "+ Finished running $*"; } 2> /dev/null -} - -function prompt { - ( - while true; do - local input="${PROMPT_ALWAYS_RESPOND:-}" - echo -n "$(tput bold)$* [Y, n]$(tput sgr0) " - [[ -z "${input}" ]] && read input - if [[ "${input}" == "Y" || "${input}" == "y" || "${input}" == "" ]]; then - return 0 - elif [[ "${input}" == "N" || "${input}" == "n" ]]; then - return 1 - fi - done - ) 2> /dev/null -} - function update_brew { DEFAULT_BREW_PATH=/usr/local/bin/brew if [ `arch` == "arm64" ] ; diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh index 60b37c3b2ad5..d44b06df22da 100755 --- a/scripts/setup-ubuntu.sh +++ b/scripts/setup-ubuntu.sh @@ -67,26 +67,6 @@ sudo --preserve-env apt update && sudo --preserve-env apt install -y libunwind-d tzdata \ wget -function run_and_time { - time "$@" - { echo "+ Finished running $*"; } 2> /dev/null -} - -function prompt { - ( - while true; do - local input="${PROMPT_ALWAYS_RESPOND:-}" - echo -n "$(tput bold)$* [Y, n]$(tput sgr0) " - [[ -z "${input}" ]] && read input - if [[ "${input}" == "Y" || "${input}" == "y" || "${input}" == "" ]]; then - return 0 - elif [[ "${input}" == "N" || "${input}" == "n" ]]; then - return 1 - fi - done - ) 2> /dev/null -} - function install_fmt { github_checkout fmtlib/fmt "${FMT_VERSION}" cmake_install -DFMT_TEST=OFF @@ -155,4 +135,4 @@ function install_velox_deps { fi ) -echo "All deps for Velox installed! Now try \"make\"" +echo "All dependencies for Velox installed!" From a7ac9a716fb30312ae4022bbca1b20c5291377c0 Mon Sep 17 00:00:00 2001 From: Pedro Pedreira Date: Wed, 14 Feb 2024 16:07:16 -0800 Subject: [PATCH 22/38] Add `VELOX_BUILD_VECTOR_TEST_UTILS` compile flag (#8747) Summary: Adding `VELOX_BUILD_VECTOR_TEST_UTILS` to allow users to compile only vector util test utilities (VectorMaker), but without bringing the larger exec test dependencies. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8747 Reviewed By: Yuhta Differential Revision: D53775689 Pulled By: pedroerp fbshipit-source-id: e43e63e28720d322ec912339c5f0d7dd8c8f0fd8 --- CMakeLists.txt | 1 + velox/vector/CMakeLists.txt | 2 +- velox/vector/tests/utils/CMakeLists.txt | 3 +-- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e2099787a969..82fed089f27c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -104,6 +104,7 @@ option(VELOX_ENABLE_CCACHE "Use ccache if installed." ON) option(VELOX_ENABLE_CODEGEN_SUPPORT "Enable experimental codegen support." OFF) option(VELOX_BUILD_TEST_UTILS "Builds Velox test utilities" OFF) +option(VELOX_BUILD_VECTOR_TEST_UTILS "Builds Velox vector test utilities" OFF) option(VELOX_BUILD_PYTHON_PACKAGE "Builds Velox Python bindings" OFF) option( VELOX_ENABLE_INT64_BUILD_PARTITION_BOUND diff --git a/velox/vector/CMakeLists.txt b/velox/vector/CMakeLists.txt index faa906605cd0..1b38cc1d248d 100644 --- a/velox/vector/CMakeLists.txt +++ b/velox/vector/CMakeLists.txt @@ -37,7 +37,7 @@ add_subdirectory(fuzzer) if(${VELOX_BUILD_TESTING}) add_subdirectory(tests) -elseif(${VELOX_BUILD_TEST_UTILS}) +elseif(${VELOX_BUILD_TEST_UTILS} OR ${VELOX_BUILD_VECTOR_TEST_UTILS}) add_subdirectory(tests/utils) endif() diff --git a/velox/vector/tests/utils/CMakeLists.txt b/velox/vector/tests/utils/CMakeLists.txt index 8b7d335db7ab..5b77b40ba99b 100644 --- a/velox/vector/tests/utils/CMakeLists.txt +++ b/velox/vector/tests/utils/CMakeLists.txt @@ -13,5 +13,4 @@ # limitations under the License. add_library(velox_vector_test_lib VectorMaker.cpp VectorTestBase.cpp) -target_link_libraries(velox_vector_test_lib velox_exec velox_vector gtest - gtest_main) +target_link_libraries(velox_vector_test_lib velox_vector gtest gtest_main) From 2068b95a31db717b81ce5903f23782522996f037 Mon Sep 17 00:00:00 2001 From: Kevin Wilfong Date: Wed, 14 Feb 2024 16:08:42 -0800 Subject: [PATCH 23/38] Call unsafeResize in FlatMapReaders (#8129) Summary: X-link: https://github.com/facebookexternal/alpha/pull/15 Pull Request resolved: https://github.com/facebookincubator/velox/pull/8129 We need to call unsafeResize in the DWRF FlatMapReader. Reviewed By: zzhao0 Differential Revision: D52339055 fbshipit-source-id: 5ce8b38cebab5da7623c8a4923dc816cd94e5e12 --- velox/dwio/dwrf/reader/FlatMapColumnReader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/velox/dwio/dwrf/reader/FlatMapColumnReader.cpp b/velox/dwio/dwrf/reader/FlatMapColumnReader.cpp index 7ba27932222b..2e52f05081b8 100644 --- a/velox/dwio/dwrf/reader/FlatMapColumnReader.cpp +++ b/velox/dwio/dwrf/reader/FlatMapColumnReader.cpp @@ -686,7 +686,7 @@ void FlatMapStructEncodingColumnReader::next( if (rowVector) { // Track children vectors in a local variable because readNulls may reset // the parent vector. - result->resize(numValues, false); + rowVector->unsafeResize(numValues, false); children = rowVector->children(); DWIO_ENSURE_EQ(children.size(), keyNodes_.size()); } From 36071eb3b7befcf97f9d7d2948f026001cd4014b Mon Sep 17 00:00:00 2001 From: lingbin Date: Thu, 15 Feb 2024 08:05:20 -0800 Subject: [PATCH 24/38] Fix typos in Memory Management documentation (#8756) Summary: `SizeClass::SizeMix` --> `MemoryAllocator::SizeMix`. Pull Request resolved: https://github.com/facebookincubator/velox/pull/8756 Reviewed By: Yuhta Differential Revision: D53810704 Pulled By: mbasmanova fbshipit-source-id: 35525ec3a165784e2b31d607c31f6ec139de4925 --- velox/docs/develop/memory.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/velox/docs/develop/memory.rst b/velox/docs/develop/memory.rst index 04e5a649f1e2..d784a4c91e1b 100644 --- a/velox/docs/develop/memory.rst +++ b/velox/docs/develop/memory.rst @@ -722,7 +722,7 @@ allocation of a fixed size buffer (class page) which is a power of 2 of a machine page size. *MMapAllocator* creates 9 different *SizeClass* objects with class page size ranging from 1 machine page (4KB) to 256 machine pages (1MB). To allocate a large number of machines pages, *MmapAllocator* calls -&MemoryAllocator::allocationSize* to build the allocation plan +*MemoryAllocator::allocationSize* to build the allocation plan (*MemoryAllocator::SizeMix*) which consists of a list of chosen *SizeClass* objects and the number of class pages to allocate from each of them. @@ -780,9 +780,9 @@ The simplified *MmapAllocator::allocateNonContiguous* implementation: #. calls *MemoryAllocator::allocationSize* with *numPages* and *minSizeClass*. *numPages* specifies the number of machine pages to allocate. *minSizeClass* specifies the minimum class page size to allocate from. The function returns - the number of class pages to allocate from each chosen *SizeClasses* in - *SizeClass::SizeMix*. The sum of machine pages to allocate from *SizeClasses* - should be no less than the requested *numPages* + the number of class pages to allocate from each chosen *SizeClass* in + *MemoryAllocator::SizeMix*. The sum of machine pages to allocate from all + *SizeClass* objects should be no less than the requested *numPages*. #. increase the memory allocator’s memory usage and check if it exceeds the system memory limit (*MemoryAllocator::capacity_*). If it exceeds, then fails @@ -901,4 +901,4 @@ check the system memory usage periodically. Whenever the system memory usage exceeds a certain threshold, it tries to free up memory from Velox by shrinking the file cache (*AsyncDataCache::shrink*), and returns the freed cache memory back to the OS. This way we can automatically shrink the file cache in response -to the transient spiky memory usage from non-Velox components in a query system. \ No newline at end of file +to the transient spiky memory usage from non-Velox components in a query system. From 615af5158b57cf1593fbe69a1c365fc44aede396 Mon Sep 17 00:00:00 2001 From: rrando901 Date: Thu, 15 Feb 2024 08:06:02 -0800 Subject: [PATCH 25/38] Add Decimal support to set_agg and set_union (#7936) Summary: Delivers https://github.com/facebookincubator/velox/issues/7935 Pull Request resolved: https://github.com/facebookincubator/velox/pull/7936 Reviewed By: mbasmanova Differential Revision: D53782978 Pulled By: Yuhta fbshipit-source-id: 435af3f6f6115d7349a99ad6338e975574413849 --- .../prestosql/aggregates/SetAggregates.cpp | 26 ++- .../prestosql/aggregates/tests/SetAggTest.cpp | 186 ++++++++++++++++++ .../aggregates/tests/SetUnionTest.cpp | 177 +++++++++++++++++ 3 files changed, 381 insertions(+), 8 deletions(-) diff --git a/velox/functions/prestosql/aggregates/SetAggregates.cpp b/velox/functions/prestosql/aggregates/SetAggregates.cpp index 4f3d3eb65ab1..c83846884fb4 100644 --- a/velox/functions/prestosql/aggregates/SetAggregates.cpp +++ b/velox/functions/prestosql/aggregates/SetAggregates.cpp @@ -385,9 +385,9 @@ class SetUnionAggregate : public SetBaseAggregate { template