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

[FLINK-33455] Implement restore tests for SortLimit node #23660

Closed
wants to merge 3 commits into from

Conversation

bvarghese1
Copy link
Contributor

What is the purpose of the change

Implement restore tests for SortLimit node

Verifying this change

This change added tests and can be verified as follows:

  • Added restore tests for SortLimit node which verifies the generated compiled plan with the saved compiled plan

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: (yes / no / don't know)
  • 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)

@flinkbot
Copy link
Collaborator

flinkbot commented Nov 3, 2023

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 Nov 7, 2023

@flinkbot run azure

@dawidwys
Copy link
Contributor

dawidwys commented Nov 7, 2023

One more thing I just noticed (actually it's also a problem for Calc tests) that we should not put those classes/tests in org.apache.flink.table.planner.plan.nodes.exec.testutils. This package is fine for RestoreTestBase as it is a testutils, but concrete tests don't fit in here.

I don't have a concrete proposal yet, but we should think of a better structure.

@bvarghese1
Copy link
Contributor Author

One more thing I just noticed (actually it's also a problem for Calc tests) that we should not put those classes/tests in org.apache.flink.table.planner.plan.nodes.exec.testutils. This package is fine for RestoreTestBase as it is a testutils, but concrete tests don't fit in here.

I don't have a concrete proposal yet, but we should think of a better structure.

Can we reuse org.apache.flink.table.planner.plan.nodes.exec.stream since we are removing tests from that package?

@dawidwys
Copy link
Contributor

dawidwys commented Nov 8, 2023

Can we reuse org.apache.flink.table.planner.plan.nodes.exec.stream since we are removing tests from that package?

Yes, let's do that

@jnh5y
Copy link
Contributor

jnh5y commented Nov 8, 2023

Can we reuse org.apache.flink.table.planner.plan.nodes.exec.stream since we are removing tests from that package?

Yes, let's do that

Should the Programs and Tests go into separate packages?

@dawidwys
Copy link
Contributor

dawidwys commented Nov 9, 2023

Should the Programs and Tests go into separate packages?

For the time being lets keep them in the same package so that we don't overcompilcate things.

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.

Good job! Looks really good!

@dawidwys dawidwys closed this in 32d31cb Nov 14, 2023
@bvarghese1 bvarghese1 deleted the sort_restore_tests branch November 14, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants