Skip to content

[FLINK-39952][Table SQL/Planner] Fix CallContextMock.isArgumentNull() to correctly handle optional arguments#28475

Open
KenanAdel wants to merge 4 commits into
apache:masterfrom
KenanAdel:FLINK-39952
Open

[FLINK-39952][Table SQL/Planner] Fix CallContextMock.isArgumentNull() to correctly handle optional arguments#28475
KenanAdel wants to merge 4 commits into
apache:masterfrom
KenanAdel:FLINK-39952

Conversation

@KenanAdel

@KenanAdel KenanAdel commented Jun 17, 2026

Copy link
Copy Markdown

What is the purpose of the change

This pull request fixes CallContextMock to better match the CallContext contract when optional arguments are not provided in test scenarios.
Previously, some methods could access argument metadata and values using direct list lookups, which could lead to IndexOutOfBoundsException when a test declared optional arguments but did not provide corresponding entries. This change makes the mock handle missing optional arguments consistently and safely.

Brief change log

  • Fixed isArgumentNull() to correctly handle missing optional arguments.
  • Added bounds checks when accessing argument metadata and values in CallContextMock (including isArgumentLiteral() and getArgumentValue()).
  • Prevented IndexOutOfBoundsException for optional arguments that are not provided.
  • Preserved existing behavior for explicitly supplied arguments.

Verifying this change

This change is already covered by existing tests.
The affected behavior is exercised through the existing type inference test infrastructure that uses CallContextMock, including tests based on InputTypeStrategiesTestBase.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude- Opus 4.7

… to correctly handle optional arguments

Prevent IndexOutOfBoundsException in CallContextMock when checking for optional arguments that are not provided in the test case. Updated the method to safely return true when the requested position is out of bounds or the underlying list is null, ensuring compliance with the CallContext interface contract.
@flinkbot

flinkbot commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@KenanAdel

Copy link
Copy Markdown
Author

@flinkbot run azure

@spuru9 spuru9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you make the PR Description as per the template provided in the repo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isArgumentLiteral() and getArgumentValue() still do a raw .get(pos). Not part of the ticket, but might as well guard it the same way while you're here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@spuru9 Done. I've updated the PR description using the repository template and added consistent guard conditions to both isArgumentLiteral() and getArgumentValue(). Thanks for the review!

@github-actions github-actions Bot added the community-reviewed PR has been reviewed by the community. label Jun 18, 2026
…BoundsException

- Update isArgumentLiteral() and getArgumentValue() to safely check for nullability and bounds, matching the logic in isArgumentNull().
- Address PR review comments for codebase consistency.
@KenanAdel KenanAdel requested a review from spuru9 June 18, 2026 14:32
@MartijnVisser

MartijnVisser commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

This is also a PR that I highly suspect that's not follow the AI contribution guidelines, and we can't merge it as is. Please follow the contribution guidelines as a whole before opening PRs @KenanAdel

@KenanAdel

Copy link
Copy Markdown
Author

Thanks for the feedback I've reviewed the AI contribution guidelines and updated the PR to comply with them, including the required disclosure and PR template updates. I have verified the implementation and understand the design and code logic The PR is ready for review. @MartijnVisser

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

Labels

community-reviewed PR has been reviewed by the community.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants