Skip to content

Conversation

jnh5y
Copy link
Contributor

@jnh5y jnh5y commented Nov 28, 2023

What is the purpose of the change

Implement restore tests for MatchRecognize node

Verifying this change

This change added tests and can be verified as follows:

  • Added restore tests for MatchRecognize 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: (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)

@flinkbot
Copy link
Collaborator

flinkbot commented Nov 28, 2023

CI report:

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

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.

Looks good. Added suggestion to extend test coverage.

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.

LGTM % the ids issue, I'll merge the PR after #23869 which is necessary for the cleanup.

.build();

static final TableTestProgram COMPLEX_MATCH =
TableTestProgram.of("complex-match", "complex match recognize test")
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 adapt the id according to the javadoc:

* The identifier must start with the name of the exec node under testing.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes; didn't see that. I was reusing identifiers from the existing tests!

Updated.

@jnh5y
Copy link
Contributor Author

jnh5y commented Dec 5, 2023

LGTM % the ids issue, I'll merge the PR after #23869 which is necessary for the cleanup.

FWIW, MR observes the changelog not final results. I think it should be ok to merge before or after the Join PR.

@dawidwys
Copy link
Contributor

dawidwys commented Dec 5, 2023

FWIW, MR observes the changelog not final results. I think it should be ok to merge before or after the Join PR.

I don't think it makes a difference. Both access static variables.

@dawidwys dawidwys merged commit 60cc00e into apache:master Dec 6, 2023
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