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): expression's error handling #17844

Closed
st1page opened this issue Jul 29, 2024 · 9 comments · Fixed by #19562
Closed

feat(batch): expression's error handling #17844

st1page opened this issue Jul 29, 2024 · 9 comments · Fixed by #19562
Labels
component/func-expr Support a SQL function or operator help wanted Issues that need help from contributors no-issue-activity

Comments

@st1page
Copy link
Contributor

st1page commented Jul 29, 2024

Some users complains that they do not want some rows' expression error prevent the whole query's processing and result.

TRY() function

presto and trino do this
https://trino.io/docs/current/functions/conditional.html#try
https://prestodb.io/docs/current/functions/conditional.html#try

NOTE: after #12461, the rw's implementation of eval_infallible is not equal to the try() function in these systems

implement your own try version of each function, such as TRY_TO_DATE(), TRY_CAST().

duckdb, snowflake and databricks do this
https://duckdb.org/docs/sql/expressions/cast.html#try_cast (though it does not have other functions)
https://docs.snowflake.com/en/sql-reference/functions/try_to_date
https://docs.databricks.com/en/sql/language-manual/functions/try_cast.html

Personally, prefer the first one.

@github-actions github-actions bot added this to the release-1.11 milestone Jul 29, 2024
@lmatz
Copy link
Contributor

lmatz commented Jul 29, 2024

I guess they chose the second one because their expression framework cannot be easily adapted for the first one 🤔
The first one is composable, I believe strictly better than the second one.

@xiangjinwu
Copy link
Contributor

Previous issue: #4458

@st1page
Copy link
Contributor Author

st1page commented Jul 29, 2024

Previous issue: #4458

Thanks! I remember we did some discussion somewhere but I did not find it.
So I think the previous issue is closed because we solved the streaming part but currently we can think about if we need to do something for the batch query.

@BugenZhao
Copy link
Member

Some users complains that they do not want some

...? 👀

@st1page
Copy link
Contributor Author

st1page commented Jul 30, 2024

Some users complains that they do not want some

...? 👀

added.. Sorry, I lost this part

@fuyufjh fuyufjh removed this from the release-2.0 milestone Aug 19, 2024
@fuyufjh
Copy link
Member

fuyufjh commented Sep 4, 2024

As a streaming database, I hope to keep the SQL features consistent among streaming (i.e. create materialized view as select) and batch (i.e. select). For this case, I think you don't plan to add TRY() to streaming, right? If so, it breaks the aforementioned consistency.

Alternatively, shall we introduce a session variable to expose the expression's strict_mode to user? This was designed in #4625 (See "Make it configurable") but not implemented yet.

@xxchan
Copy link
Member

xxchan commented Sep 5, 2024

Alternatively, shall we introduce a session variable to expose the expression's strict_mode to user? This was designed in #4625 (See "Make it configurable") but not implemented yet.

IIUC, currently

  • batch: only strict mode
  • streaming: only non-strict mode

Do you mean to support non-strict mode for batch via a session var? We can't support strict mode for streaming easily. (Perhaps better to call it batch_strict_mode)

@fuyufjh
Copy link
Member

fuyufjh commented Sep 5, 2024

Do you mean to support non-strict mode for batch via a session var? We can't support strict mode for streaming easily.

Exactly.

It makes sense to use strict mode for batch query, so that unexpected problems can be revealed early to prompt users to modify their query or clean their data. But in some cases, users may expect a failed expression to leave a NULL as result, so they can explicitly set strict_mode = false to work around.

(Perhaps better to call it batch_strict_mode)

Yeah, agree. But I'd like to mention 'expression' as well, how about batch_expr_strict_mode?

@fuyufjh fuyufjh added help wanted Issues that need help from contributors component/func-expr Support a SQL function or operator labels Sep 5, 2024
Copy link
Contributor

github-actions bot commented Nov 5, 2024

This issue has been open for 60 days with no activity.

If you think it is still relevant today, and needs to be done in the near future, you can comment to update the status, or just manually remove the no-issue-activity label.

You can also confidently close this issue as not planned to keep our backlog clean.
Don't worry if you think the issue is still valuable to continue in the future.
It's searchable and can be reopened when it's time. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/func-expr Support a SQL function or operator help wanted Issues that need help from contributors no-issue-activity
Projects
None yet
6 participants