Fix partial aggregation #10

mbasmanova · 2021-08-09T21:41:34Z

HashAggregation operator could get into an invalid state where it is not
blocked, doesn't need input, but getOutput() returns null. This happened when
operator processed a "distinct" aggregation and reached a limit on the amount
of memory to use for partial aggregation. This state confused the Driver loop
and caused it to complete prematurely. This would result in either wrong
results or a crash.

The fix is to flush partial aggregation state when reached the limit.

HashAggregation operator could get into an invalid state where it is not blocked, doesn't need input, but getOutput() returns null. This happened when operator processed a "distinct" aggregation and reached a limit on the amount of memory to use for partial aggregation. This state confused the Driver loop and caused it to complete prematurely. This would result in either wrong results or a crash. The fix is to flush partial aggregation state when reached the limit.

facebook-github-bot · 2021-08-09T21:41:46Z

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

facebook-github-bot · 2021-08-10T02:43:12Z

@mbasmanova merged this pull request in e06d004.

* Rename some components * More fixes * Fix compile issue * Minor fix

…ator#10) * remove arrow datasource * remove the dependency on log4j

This reverts commit fe3002e.

…10195) Summary: Add support for DECIMAL input to greatest and least Spark functions. Spark adds cast to handle different decimal types during planning, so we don't need to handle the type difference in functions. https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala#L482 Example: ``` spark.sql(""" SELECT * FROM VALUES ( CAST(5.345 AS DECIMAL(6, 2)), CAST(5.35 AS DECIMAL(5, 4)))) AS data(a, b); """).write.mode("overwrite").parquet("decimal_table") spark.read.parquet("decimal_table").createOrReplaceTempView("decimal_table") val df = spark.sql("SELECT greatest(a, b) from decimal_table") ``` `Project [greatest(cast(a#6 as decimal(8,4)), cast(b#7 as decimal(8,4))) AS greatest(a, b)https://github.com/facebookincubator/velox/issues/10]` Pull Request resolved: #10195 Reviewed By: pedroerp Differential Revision: D58825799 Pulled By: bikramSingh91 fbshipit-source-id: 370906f010e70a3329fb14cd14fb8e10fdad40da

Summary: The default algorithm used is MD5. However, MD5 is not supported with fips and can cause a SIGSEGV. Set CRC32 instead which is a standard for checksum computation and is not restricted by fips. crc32 is also faster than md5. Internally at IBM, we hit the following SIGSEGV ``` 0x0000000000000000 in ?? () Missing separate debuginfos, use: dnf debuginfo-install openssl-fips-provider-3.0.7-2.el9.x86_64 xz-libs-5.2.5-8.el9_0.x86_64 (gdb) bt #0 0x0000000000000000 in ?? () #1 0x0000000004e5f89b in Aws::Utils::Crypto::MD5OpenSSLImpl::Calculate(std::istream&) () #2 0x0000000004efd298 in Aws::Utils::Crypto::MD5::Calculate(std::istream&) () #3 0x0000000004ef71b9 in Aws::Utils::HashingUtils::CalculateMD5(std::iostream&) () #4 0x0000000004e8ebe8 in Aws::Client::AWSClient::AddChecksumToRequest(std::shared_ptr<Aws::Http::HttpRequest> const&, Aws::AmazonWebServiceRequest const&) const () #5 0x0000000004e8ed15 in Aws::Client::AWSClient::BuildHttpRequest(Aws::AmazonWebServiceRequest const&, std::shared_ptr<Aws::Http::HttpRequest> const&) const () #6 0x0000000004e977f9 in Aws::Client::AWSClient::AttemptOneRequest(std::shared_ptr<Aws::Http::HttpRequest> const&, Aws::AmazonWebServiceRequest const&, char const*, char const*, char const*) const () #7 0x0000000004e9e1c0 in Aws::Client::AWSClient::AttemptExhaustively(Aws::Http::URI const&, Aws::AmazonWebServiceRequest const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const () #8 0x0000000004ea15e8 in Aws::Client::AWSXMLClient::MakeRequest(Aws::Http::URI const&, Aws::AmazonWebServiceRequest const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const () #9 0x0000000004ea1f70 in Aws::Client::AWSXMLClient::MakeRequest(Aws::AmazonWebServiceRequest const&, Aws::Endpoint::AWSEndpoint const&, Aws::Http::HttpMethod, char const*, char const*, char const*) const () #10 0x0000000004de0933 in Aws::S3::S3Client::UploadPart(Aws::S3::Model::UploadPartRequest const&) const::{lambda()https://github.com/facebookincubator/velox/issues/1}::operator()() const () #11 0x0000000004de0b8c in std::_Function_handler<Aws::Utils::Outcome<Aws::S3::Model::UploadPartResult, Aws::S3::S3Error> (), Aws::S3::S3Client::UploadPart(Aws::S3::Model::UploadPartRequest const&) const::{lambda()https://github.com/facebookincubator/velox/issues/1}>::_M_invoke(std::_Any_data const&) () #12 0x0000000004e19317 in Aws::Utils::Outcome<Aws::S3::Model::UploadPartResult, Aws::S3::S3Error> smithy::components::tracing::TracingUtils::MakeCallWithTiming<Aws::Utils::Outcome<Aws::S3::Model::UploadPartResult, Aws::S3::S3Error> >(std::function<Aws::Utils::Outcome<Aws::S3::Model::UploadPartResult, Aws::S3::S3Error> ()>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, smithy::components::tracing::Meter const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) () #13 0x0000000004d7cdcf in Aws::S3::S3Client::UploadPart(Aws::S3::Model::UploadPartRequest const&) const () #14 0x0000000004ca4aa6 in facebook::velox::filesystems::S3WriteFile::Impl::uploadPart (this=0x7fffec2f09a0, part=..., isLast=true) at /root/velox/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp:380 ``` Pull Request resolved: #10801 Reviewed By: amitkdutta Differential Revision: D61671574 Pulled By: kgpai fbshipit-source-id: 34c7b777b3fde0659ef74c4fbfd93740fdfa3f7c

Summary: A minor CMake fix for #11609, to address the linking issue when `VELOX_BUILD_TESTING` is off. ```C++ 20:52:36 #10 2.268 CMake Error at velox/velox/runner/tests/CMakeLists.txt:19 (target_link_libraries): 20:52:36 #10 2.268 Target "velox_local_runner_test" links to: 20:52:36 #10 2.268 20:52:36 #10 2.268 GTest::gtest 20:52:36 #10 2.268 20:52:36 #10 2.268 but the target was not found. Possible reasons include: 20:52:36 #10 2.268 20:52:36 #10 2.268 * There is a typo in the target name. 20:52:36 #10 2.268 * A find_package call is missing for an IMPORTED target. 20:52:36 #10 2.268 * An ALIAS target is missing. 20:52:36 #10 2.268 20:52:36 #10 2.268 20:52:36 #10 2.268 20:52:36 #10 2.273 CMake Generate step failed. Build files cannot be regenerated correctly. 20:52:36 #10 2.302 make: *** [Makefile:96: cmake-and-build] Error 1 20:52:36 #10 2.302 make: Leaving directory '/prestissimo' 20:52:36 #10 ERROR: process "/bin/sh -c EXTRA_CMAKE_FLAGS=${EXTRA_CMAKE_FLAGS} NUM_THREADS=${NUM_THREADS} make --directory=\"/prestissimo/\" cmake-and-build BUILD_TYPE=${BUILD_TYPE} BUILD_DIR=${BUILD_DIR} BUILD_BASE_DIR=${BUILD_BASE_DIR}" did not complete successfully: exit code: 2 ``` Pull Request resolved: #11669 Reviewed By: zacw7 Differential Revision: D66560342 Pulled By: xiaoxmeng fbshipit-source-id: 9f6b74c5a64f6e248cb8d86ec213ae76eb894770

…kincubator#11669) Summary: A minor CMake fix for facebookincubator#11609, to address the linking issue when `VELOX_BUILD_TESTING` is off. ```C++ 20:52:36 facebookincubator#10 2.268 CMake Error at velox/velox/runner/tests/CMakeLists.txt:19 (target_link_libraries): 20:52:36 facebookincubator#10 2.268 Target "velox_local_runner_test" links to: 20:52:36 facebookincubator#10 2.268 20:52:36 facebookincubator#10 2.268 GTest::gtest 20:52:36 facebookincubator#10 2.268 20:52:36 facebookincubator#10 2.268 but the target was not found. Possible reasons include: 20:52:36 facebookincubator#10 2.268 20:52:36 facebookincubator#10 2.268 * There is a typo in the target name. 20:52:36 facebookincubator#10 2.268 * A find_package call is missing for an IMPORTED target. 20:52:36 facebookincubator#10 2.268 * An ALIAS target is missing. 20:52:36 facebookincubator#10 2.268 20:52:36 facebookincubator#10 2.268 20:52:36 facebookincubator#10 2.268 20:52:36 facebookincubator#10 2.273 CMake Generate step failed. Build files cannot be regenerated correctly. 20:52:36 facebookincubator#10 2.302 make: *** [Makefile:96: cmake-and-build] Error 1 20:52:36 facebookincubator#10 2.302 make: Leaving directory '/prestissimo' 20:52:36 facebookincubator#10 ERROR: process "/bin/sh -c EXTRA_CMAKE_FLAGS=${EXTRA_CMAKE_FLAGS} NUM_THREADS=${NUM_THREADS} make --directory=\"/prestissimo/\" cmake-and-build BUILD_TYPE=${BUILD_TYPE} BUILD_DIR=${BUILD_DIR} BUILD_BASE_DIR=${BUILD_BASE_DIR}" did not complete successfully: exit code: 2 ``` Pull Request resolved: facebookincubator#11669 Reviewed By: zacw7 Differential Revision: D66560342 Pulled By: xiaoxmeng fbshipit-source-id: 9f6b74c5a64f6e248cb8d86ec213ae76eb894770

mbasmanova requested review from kgpai, pedroerp and oerling August 9, 2021 21:41

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

mbasmanova mentioned this pull request Aug 9, 2021

Fix Driver loop #9

Closed

facebook-github-bot closed this in e06d004 Aug 10, 2021

facebook-github-bot added the Merged label Aug 10, 2021

pedroerp mentioned this pull request Aug 18, 2021

fromUnixtime() crashes if it gets a NaN or Inf #67

Closed

winningsix pushed a commit to winningsix/velox-1 that referenced this pull request Sep 27, 2021

Rename some components for execution part (facebookincubator#10)

6eaa72d

* Rename some components * More fixes * Fix compile issue * Minor fix

winningsix pushed a commit to winningsix/velox-1 that referenced this pull request Sep 27, 2021

Rename some components for execution part (facebookincubator#10)

b4724cb

* Rename some components * More fixes * Fix compile issue * Minor fix

zhouyuan pushed a commit to zhouyuan/velox that referenced this pull request May 11, 2022

Fix agg step when only groupings exist (facebookincubator#10)

cbd85fd

This was referenced Oct 8, 2022

from_unixtime() crashes with error: nan is outside the range of representable values of type 'long' #2791

Closed

Substrait velox_plan_conversion_test is flaky #2816

Closed

rui-mo added a commit to rui-mo/velox that referenced this pull request Mar 17, 2023

[GJ-9] Clean up arrow data source and log4j dependency (facebookincub…

aabe06c

…ator#10) * remove arrow datasource * remove the dependency on log4j

rui-mo added a commit to rui-mo/velox that referenced this pull request Mar 22, 2023

fix incorrect is-decimal check (facebookincubator#10)

fe3002e

rui-mo added a commit to rui-mo/velox that referenced this pull request Mar 22, 2023

Revert "fix incorrect is-decimal check (facebookincubator#10)"

8da8159

This reverts commit fe3002e.

billyean mentioned this pull request Jun 7, 2023

Fix velox_aggregates_test memory leaks #5168

Closed

mayunlei mentioned this pull request Jun 7, 2023

Coredump caused by modifying the same vector in multiple Driver threads #5169

Open

heary-cloud mentioned this pull request Dec 14, 2023

Crash in ExchangeClient::request on acquiring a lock on queue_->mutex #8044

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix partial aggregation #10

Fix partial aggregation #10

mbasmanova commented Aug 9, 2021

facebook-github-bot commented Aug 9, 2021

facebook-github-bot commented Aug 10, 2021

Fix partial aggregation #10

Fix partial aggregation #10

Conversation

mbasmanova commented Aug 9, 2021

facebook-github-bot commented Aug 9, 2021

facebook-github-bot commented Aug 10, 2021