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

feat(batch): add batch_expr_strict_mode to ignore expression error in batch query #19562

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

fuyufjh
Copy link
Member

@fuyufjh fuyufjh commented Nov 25, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Resolves #17844. See #17844 (comment).

Currently, the error is printed to logs only (with LogSuppressor to avoid flooding). It would be better to notice to users via psql's NOTICE, but it requires more work to pass these messages from compute nodes to frontend nodes. Left a TODO in the code now.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

  • Added a session variable batch_expr_strict_mode to control whether to let the entire query fail or fill NULL values for expression evaluation failures.

Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

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

LGTM!

@fuyufjh fuyufjh added the user-facing-changes Contains changes that are visible to users label Nov 26, 2024
Copy link
Contributor

Hi, there.

📝 Telemetry Reminder:
If you're implementing this feature, please consider adding telemetry metrics to track its usage. This helps us understand how the feature is being used and improve it further.
You can find the function report_event of telemetry reporting in the following files. Feel free to ask questions if you need any guidance!

  • src/frontend/src/telemetry.rs
  • src/meta/src/telemetry.rs
  • src/stream/src/telemetry.rs
  • src/storage/compactor/src/telemetry.rs
    Or calling report_event_common (src/common/telemetry_event/src/lib.rs) as if finding it hard to implement.
    ✨ Thank you for your contribution to RisingWave! ✨

This is an automated comment created by the peaceiris/actions-label-commenter. Responding to the bot or mentioning it won't have any effect.

@fuyufjh fuyufjh added this pull request to the merge queue Nov 27, 2024
Merged via the queue into main with commit ba76431 Nov 27, 2024
33 of 34 checks passed
@fuyufjh fuyufjh deleted the eric/batch_non_strict_expr_eval branch November 27, 2024 03:07
@@ -17,7 +17,7 @@ use risingwave_common::array::ArrayImpl::Bool;
use risingwave_common::array::DataChunk;
use risingwave_common::catalog::Schema;
use risingwave_common::util::chunk_coalesce::DataChunkBuilder;
use risingwave_expr::expr::{build_from_prost, BoxedExpression};
use risingwave_expr::expr::{build_batch_expr_from_prost, BoxedExpression};
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that build_from_prost will not be directly used anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is still used quite a bit. Only Filter and Project have been changed to build_batch_expr_from_prost. Examples of the rest that still uses build_from_prost include

  • HopWindowExecutor: because the expression is derived from +/- interval, so it must be non-fallable
  • SortAggExecutor: same as above, but it's even simpler - there are only ValueRefs
  • UpdateExecutor: because I can't image why users need to ignore expressions here, so I just keep it unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(batch): expression's error handling
3 participants