Skip to content

[FLINK-37721] Fixes janino bug returning incorrect results #26504

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

Merged
merged 1 commit into from
May 13, 2025

Conversation

AlanConfluent
Copy link
Contributor

@AlanConfluent AlanConfluent commented Apr 24, 2025

What is the purpose of the change

The bug comes from the fact that previously, we were creating an anonymous function callback within the janino generated code. This function was then referencing the local DelegatingAsyncResultFuture, effectively creating a closure. When the function was called back from different threads, they seemed to both reference the same DelegatingAsyncResultFuture rather than their own local ones. The result was returning incorrect "synchronous" results, so queries like the following might have issues:

SELECT f1, func(f1, f2, f3) FROM Table1;

Here, the f1 column returned was retrieved by asking for it from the local DelegatingAsyncResultFuture, and so getting the wrong instance meant getting the wrong result.

To fix it, I removed the inner anonymous function and just added the same logic on DelegatingAsyncResultFuture. It requires setting it up with all of the metadata (indexes, sync results), and when the result is complete, rather than calling the generated function, it just creates the result RowData itself.

Brief change log

  • Changes how codegen works for async calcs to avoid anonymous functions

Verifying this change

This change added tests and can be verified as follows:

  • Added ITCase that previously gave incorrect result, but which now gives the right result.
  • Added test which shows that changelog type is propagated

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

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
    • The former generated code and static code in DelegatingAsyncResultFuture shouldn't differ in any significant way, so should be a no-op.
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

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

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 24, 2025

CI report:

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

@dawidwys
Copy link
Contributor

dawidwys commented May 6, 2025

Could you please fix the PR to meet the community standards? In particular:

  1. Please add jira number to each commit and the PR title.
  2. Please fill in the PR template

You can find contribution guidelines here: https://flink.apache.org/how-to-contribute/code-style-and-quality-pull-requests/

Thank you

Copy link
Contributor

@dawidwys dawidwys left a comment

Choose a reason for hiding this comment

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

The PR should be good after fixing the PR requirements

Comment on lines 245 to 246
// This was the cause of a bug previously where the reference to the sync projection was
// getting garbled by janino.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please link the JIRA issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, added the link

Copy link
Contributor

@dawidwys dawidwys May 12, 2025

Choose a reason for hiding this comment

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

Could you please double check you committed the change. I don't see a JIRA number mentioned in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. I included the link this time.

@AlanConfluent AlanConfluent changed the title Fixes janino bug returning incorrect results [FLINK-37721] Fixes janino bug returning incorrect results May 9, 2025
@dawidwys dawidwys merged commit ef934ca into apache:master May 13, 2025
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.

3 participants