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.

Sign up for GitHub

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

Jump to bottom

feat: Support scan filter for ORC decimal reader #11067

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Support scan filter for ORC decimal reader #11067

wants to merge 1 commit into from
+301 −13

Conversation

rui-mo
Copy link
Collaborator

@rui-mo rui-mo commented Sep 23, 2024

'leafCallToSubfieldFilter' converts decimal filter as Subfield filter, but
'SelectiveDecimalColumnReader::read' rejects scan spec filter. This PR supports
scan filter for ORC decimal reader.

@facebook-github-bot 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 Sep 23, 2024
@netlify Netlify
Copy link

netlify bot commented Sep 23, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit ed04a8e
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67acabd183a21b00080126db

@Yuhta Yuhta self-requested a review September 23, 2024 14:41
Yuhta
Yuhta reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@Yuhta Yuhta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to add some tests to E2EFilterTest for decimal types

velox/dwio/dwrf/reader/SelectiveDecimalColumnReader.cpp
// Fill decimals before applying filter.
fillDecimals();

const auto rawNulls = nullsInReadRange_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract this logic to a static function or trait class so that it can be later reused in parquet as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parquet decimal uses SelectiveIntegerColumnReader::processFilter so it might don't need to reuse this logic. How do you think? Thanks.

velox/dwio/dwrf/reader/SelectiveDecimalColumnReader.cpp Outdated
if constexpr (std::is_same_v<DataT, int64_t>) {
processFilter(filter, rows, rawNulls);
} else {
VELOX_UNSUPPORTED("Unsupported filter: {}.", (int)filterKind);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be the case for schema evolution. Just throw VELOX_FAIL here or VELOX_NYI for these and add the requested type and file type information to the error message.

@rui-mo
Copy link
Collaborator Author

rui-mo commented Sep 25, 2024

You also need to add some tests to E2EFilterTest for decimal types

@Yuhta I find the E2EFilterTest relies on DWRF writer, but for now bigint-decimal will be written as int64_t, and hugeint-decimal is not supported. Do we need to support them first?

velox/velox/dwio/dwrf/writer/ColumnWriter.cpp

Lines 2039 to 2041 in 3a1a60a

case TypeKind::BIGINT:
return std::make_unique<IntegerColumnWriter<int64_t>>(
context, type, sequence, onRecordPosition);

velox/velox/dwio/dwrf/writer/ColumnWriter.cpp

Line 2090 in 3a1a60a

DWIO_RAISE("not supported yet ", mapTypeKindToName(type.type()->kind()));

@Yuhta
Copy link
Contributor

Yuhta commented Sep 25, 2024

@rui-mo Yes it would be ideal to support writing decimals first. Otherwise the tests are very limited and we have no confidence the new code would work correctly.

@rui-mo rui-mo force-pushed the wip_decimal_filter branch from 2937bac to e1b7ef8 Compare October 10, 2024 06:39
@rui-mo rui-mo mentioned this pull request Nov 5, 2024
Open
@rui-mo rui-mo force-pushed the wip_decimal_filter branch from e1b7ef8 to 3117603 Compare November 20, 2024 03:12
@rui-mo rui-mo changed the title Support scan filter for ORC decimal reader feat: Support scan filter for ORC decimal reader Nov 20, 2024
@rui-mo rui-mo force-pushed the wip_decimal_filter branch from 3117603 to edfb582 Compare December 5, 2024 03:31
@rui-mo rui-mo force-pushed the wip_decimal_filter branch from edfb582 to 86a8f95 Compare January 24, 2025 12:35
@wypb wypb mentioned this pull request Feb 11, 2025
Open
@rui-mo rui-mo force-pushed the wip_decimal_filter branch from 86a8f95 to 1a86c34 Compare February 11, 2025 15:57
@rui-mo rui-mo marked this pull request as ready for review February 11, 2025 15:58
@rui-mo rui-mo force-pushed the wip_decimal_filter branch from 1a86c34 to 6750596 Compare February 11, 2025 16:43
@rui-mo rui-mo force-pushed the wip_decimal_filter branch from 6750596 to 6e1ad03 Compare February 11, 2025 16:55
aditi-pandit
aditi-pandit reviewed Feb 11, 2025
View reviewed changes
velox/dwio/dwrf/reader/SelectiveDecimalColumnReader.cpp
const common::Filter* filter,
const RowSet& rows,
const uint64_t* rawNulls) {
returnReaderNulls_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add VELOX_CHECK(filter) to assert the function is always called with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the below check, thanks.

VELOX_CHECK_NOT_NULL(filter, "Filter must not be null.");

@rui-mo rui-mo force-pushed the wip_decimal_filter branch from 6e1ad03 to ed04a8e Compare February 12, 2025 14:10
@rui-mo
Copy link
Collaborator Author

rui-mo commented Feb 12, 2025

@rui-mo Yes it would be ideal to support writing decimals first. Otherwise the tests are very limited and we have no confidence the new code would work correctly.

The E2E filter tests depend on decimal write support in #11431.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouyuan zhouyuan zhouyuan left review comments

@Yuhta Yuhta Yuhta left review comments

@aditi-pandit aditi-pandit aditi-pandit left review comments

@assignUser assignUser Awaiting requested review from assignUser assignUser is a code owner

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rui-mo @Yuhta @zhouyuan @aditi-pandit @facebook-github-bot