Skip to content

[CALCITE-7706] ARG_MIN ignores nullability of second argument - #5173

Merged
mihaibudiu merged 1 commit into
apache:mainfrom
mihaibudiu:issue7706
Aug 11, 2026
Merged

[CALCITE-7706] ARG_MIN ignores nullability of second argument#5173
mihaibudiu merged 1 commit into
apache:mainfrom
mihaibudiu:issue7706

Conversation

@mihaibudiu

Copy link
Copy Markdown
Contributor

CALCITE-7706

Changes Proposed

Type inference for ARG_MIN/ARG_MAX needs to check whether the second argument is nullable. It only used to check the first argument. This could lead to incorrect results or crashes at runtime.

@xuzifu666 xuzifu666 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, only add a comment for QuidemTest.

Comment thread core/src/test/resources/sql/agg.iq Outdated
# Rows whose comparator is NULL are skipped, so a group where every
# comparator value is NULL yields NULL even though the value argument
# is NOT NULL.
select g, arg_min(v, c) as r

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agg.iq only tested the runtime behavior of ARG_MIN and not ARG_MAX; although the logic is symmetrical, it would be good to add the missing test case(This revision also involves arg_max).

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Aug 11, 2026
@mihaibudiu

Copy link
Copy Markdown
Contributor Author

I have extended both quidem tests to do both ARG_MAX and ARG_MIN.
Since you have approved I will squash and merge

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud

Copy link
Copy Markdown

@mihaibudiu
mihaibudiu merged commit 018de2e into apache:main Aug 11, 2026
19 checks passed
@mihaibudiu
mihaibudiu deleted the issue7706 branch August 11, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants