Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SelectiveDecimalColumnReader::scanSpec_::valueHook_ may not be nullptr #11290

Closed
NEUpanning opened this issue Oct 17, 2024 · 2 comments
Closed
Labels
bug Something isn't working triage Newly created issue that needs attention.

Comments

@NEUpanning
Copy link
Contributor

NEUpanning commented Oct 17, 2024

Bug description

Exception is caused by SelectiveDecimalColumnReader::scanSpec_::valueHook_ != nullptr when the physical type of decimal is int64_t and aggregation push-down is enabled.

Error Source: RUNTIME
Error Code: INVALID_STATE
Retriable: False
Expression: !scanSpec_->valueHook()
Context: Split [Hive: viewfs://hadoop-meituan/ghann03/warehouse/app_hotel.db/app_poi_leopard_coverage_medal/datekey=20240910/000000_0 0 - 63768834] Task Gluten_Stage_0_TID_31_VTID_0
Additional Context: Operator: PartialAggregation[1] 1
Function: read
File: /opt/meituan/panning/gluten-dev/velox/velox/dwio/dwrf/reader/SelectiveDecimalColumnReader.cpp
Line: 115
facebook::velox::process::StackTrace::StackTrace(int)
_ZN8facebook5velox14VeloxException5State4makeIZNS1_C4EPKcmS5_St17basic_string_viewIcSt11char_traitsIcEES9_S9_S9_bNS1_4TypeES9_EUlRT_E_EESt10shared_ptrIKS2_ESA_SB_
facebook::velox::VeloxException::VeloxException(char const*, unsigned long, char const*, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, bool, facebook::velox::VeloxException::Type, std::basic_string_view<char, std::char_traits<char> >)
facebook::velox::VeloxRuntimeError::VeloxRuntimeError(char const*, unsigned long, char const*, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, bool, std::basic_string_view<char, std::char_traits<char> >)
void facebook::velox::detail::veloxCheckFail<facebook::velox::VeloxRuntimeError, facebook::velox::detail::CompileTimeEmptyString>(facebook::velox::detail::VeloxCheckFailArgs const&, facebook::velox::detail::CompileTimeEmptyString)
facebook::velox::dwrf::SelectiveDecimalColumnReader<long>::read(int, folly::Range<int const*>, unsigned long const*)
facebook::velox::dwio::common::ColumnLoader::loadInternal(folly::Range<int const*>, facebook::velox::ValueHook*, int, std::shared_ptr<facebook::velox::BaseVector>*)
facebook::velox::VectorLoader::load(folly::Range<int const*>, facebook::velox::ValueHook*, int, std::shared_ptr<facebook::velox::BaseVector>*)
facebook::velox::LazyVector::load(folly::Range<int const*>, facebook::velox::ValueHook*) const
void facebook::velox::functions::aggregate::SimpleNumericAggregate<long, long, long>::pushdown<facebook::velox::aggregate::MinMaxHook<long, false> >(char**, facebook::velox::SelectivityVector const&, std::shared_ptr<facebook::velox::BaseVector> const&)
facebook::velox::functions::aggregate::(anonymous namespace)::MaxAggregate<long>::addRawInput(char**, facebook::velox::SelectivityVector const&, std::vector<std::shared_ptr<facebook::velox::BaseVector>, std::allocator<std::shared_ptr<facebook::velox::BaseVector> > > const&, bool)
facebook::velox::exec::AggregateCompanionFunctionBase::addRawInput(char**, facebook::velox::SelectivityVector const&, std::vector<std::shared_ptr<facebook::velox::BaseVector>, std::allocator<std::shared_ptr<facebook::velox::BaseVector> > > const&, bool)
facebook::velox::exec::GroupingSet::addInputForActiveRows(std::shared_ptr<facebook::velox::RowVector> const&, bool)
facebook::velox::exec::GroupingSet::addInput(std::shared_ptr<facebook::velox::RowVector> const&, bool)
facebook::velox::exec::HashAggregation::addInput(std::shared_ptr<facebook::velox::RowVector>)
facebook::velox::exec::Driver::runInternal(std::shared_ptr<facebook::velox::exec::Driver>&, std::shared_ptr<facebook::velox::exec::BlockingState>&, std::shared_ptr<facebook::velox::RowVector>&)
facebook::velox::exec::Driver::next(std::shared_ptr<facebook::velox::exec::BlockingState>&)
facebook::velox::exec::Task::next(folly::SemiFuture<folly::Unit>*)
gluten::WholeStageResultIterator::next()
gluten::ResultIterator::getNext()
gluten::ResultIterator::hasNext()

I see that currently only int128_t doesn't enable push-down for SelectiveDecimalColumnReader, but decimal also can be int64_t. Perhaps, we should let int64_t do this as well.

System information

Velox System Info v0.0.2
Commit: 33928e00
CMake Version: 3.29.6
System: Linux-3.10.0-862.mt20190308.130.el7.x86_64
Arch: x86_64
C++ Compiler: /opt/rh/devtoolset-10/root/usr/bin/c++
C++ Compiler Version: 10.2.1
C Compiler: /opt/rh/devtoolset-10/root/usr/bin/cc
C Compiler Version: 10.2.1

Relevant logs

No response

@NEUpanning NEUpanning added bug Something isn't working triage Newly created issue that needs attention. labels Oct 17, 2024
@Yuhta
Copy link
Contributor

Yuhta commented Oct 17, 2024

int64_t needs to be enabled, we need to check in the aggregate function that !type->isDecimal() to enable pushdowns.

@NEUpanning
Copy link
Contributor Author

@Yuhta Thanks for your reply. I'd like to work on it.

weiting-chen pushed a commit to weiting-chen/velox that referenced this issue Nov 23, 2024
Summary:
Currently, `int64_t` enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes facebookincubator#11290

Pull Request resolved: facebookincubator#11298

Reviewed By: Yuhta

Differential Revision: D65834210

Pulled By: kevinwilfong

fbshipit-source-id: 422f7eda8f4184c6fa83055e7cf430ff5053d387
weiting-chen added a commit to oap-project/velox that referenced this issue Nov 23, 2024
…ebookincubator#11298 (#505)

* Fix NaN handling for min/max aggreates pushed down to scan (facebookincubator#10583)

Summary:
Pull Request resolved: facebookincubator#10583

This fixes min/max aggregates to handle NaN values correctly when they are pushed down to the scan operator. Specifically, the change ensures that NaN values are considered greater than infinity.

Reviewed By: zacw7

Differential Revision: D60297934

fbshipit-source-id: 3398ba24d6fc70bbd0d8583d2949391b0824099c

* Schema evolution support for reader value hooks (facebookincubator#10755)

Summary:
X-link: facebookincubator/nimble#72

Pull Request resolved: facebookincubator#10755

Currently reader value hook is not considering schema evolution at all, this change fix that.

Reviewed By: kevinwilfong

Differential Revision: D61229494

fbshipit-source-id: 729bb90611fb3164282b524376eda20985a30194

* Disable aggregate pushdown for decimal type (facebookincubator#11298)

Summary:
Currently, `int64_t` enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes facebookincubator#11290

Pull Request resolved: facebookincubator#11298

Reviewed By: Yuhta

Differential Revision: D65834210

Pulled By: kevinwilfong

fbshipit-source-id: 422f7eda8f4184c6fa83055e7cf430ff5053d387

---------

Co-authored-by: Bikramjeet Vig <[email protected]>
Co-authored-by: Jimmy Lu <[email protected]>
Co-authored-by: NEUpanning <[email protected]>
athmaja-n pushed a commit to athmaja-n/velox that referenced this issue Jan 10, 2025
Summary:
Currently, `int64_t` enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes facebookincubator#11290

Pull Request resolved: facebookincubator#11298

Reviewed By: Yuhta

Differential Revision: D65834210

Pulled By: kevinwilfong

fbshipit-source-id: 422f7eda8f4184c6fa83055e7cf430ff5053d387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage Newly created issue that needs attention.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants