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.

Sign up for GitHub

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

Jump to bottom

[FLINK-35014] SqlNode to operation conversion for models #25834

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[FLINK-35014] SqlNode to operation conversion for models #25834

wants to merge 6 commits into from
+2,341 −89

Conversation

lihaosky
Copy link
Contributor

What is the purpose of the change

Add SqlNode to SqlOperation conversion for models

Brief change log

  • Support Model CRUD node to operation conversion

Verifying this change

This change added tests and can be verified as follows:

  • Added unit test and integration test in TableEnvironmentTest

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): (yes)
  • 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? (JavaDocs)

@lihaosky
Copy link
Contributor Author

cc @twalthr to review. Thanks!

@flinkbot
Copy link
Collaborator

flinkbot commented Dec 20, 2024 •
edited
Loading

CI report:

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

@lihaosky
Copy link
Contributor Author 

2024-12-20T21:31:47.4797792Z Dec 20 21:31:47 21:31:47.478 [INFO] Running org.apache.flink.table.planner.plan.rules.physical.stream.WatermarkAssignerChangelogNormalizeTransposeRuleTest
2024-12-20T21:31:47.7285003Z Dec 20 21:31:47 21:31:47.726 [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.865 s <<< FAILURE! -- in org.apache.flink.architecture.rules.BanJunit4Rules
2024-12-20T21:31:47.7287086Z Dec 20 21:31:47 21:31:47.726 [ERROR] BanJunit4Rules.NO_NEW_ADDED_JUNIT4_TEST_RULE -- Time elapsed: 2.860 s <<< FAILURE!
2024-12-20T21:31:47.7288102Z Dec 20 21:31:47 java.lang.AssertionError: 
2024-12-20T21:31:47.7289605Z Dec 20 21:31:47 Architecture Violation [Priority: MEDIUM] - Rule 'Junit4 is forbidden, please use Junit5 instead' was violated (2 times):
2024-12-20T21:31:47.7291813Z Dec 20 21:31:47 Method <org.apache.flink.table.planner.operations.SqlDdlToOperationConverterTest.testCreateModel()> calls method <org.junit.Assert.assertNotNull(java.lang.Object)> in (SqlDdlToOperationConverterTest.java:296)
2024-12-20T21:31:47.7294137Z Dec 20 21:31:47 Method <org.apache.flink.table.planner.operations.SqlDdlToOperationConverterTest.testCreateModel()> calls method <org.junit.Assert.assertNotNull(java.lang.Object)> in (SqlDdlToOperationConverterTest.java:303)
2024-12-20T21:31:47.7295939Z Dec 20 21:31:47 	at com.tngtech.archunit.lang.ArchRule$Assertions.assertNoViolation(ArchRule.java:94)
2024-12-20T21:31:47.7297056Z Dec 20 21:31:47 	at com.tngtech.archunit.lang.ArchRule$Assertions.check(ArchRule.java:86)
2024-12-20T21:31:47.7298210Z Dec 20 21:31:47 	at com.tngtech.archunit.library.freeze.FreezingArchRule.check(FreezingArchRule.java:97)
2024-12-20T21:31:47.7301989Z Dec 20 21:31:47 	at com.tngtech.archunit.junit.internal.ArchUnitTestDescriptor$ArchUnitRuleDescriptor.execute(ArchUnitTestDescriptor.java:166)
2024-12-20T21:31:47.7303496Z Dec 20 21:31:47 	at com.tngtech.archunit.junit.internal.ArchUnitTestDescriptor$ArchUnitRuleDescriptor.execute(ArchUnitTestDescriptor.java:149)
2024-12-20T21:31:47.7317722Z Dec 20 21:31:47 	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
2024-12-20T21:31:47.7319141Z Dec 20 21:31:47 	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
2024-12-20T21:31:47.7320302Z Dec 20 21:31:47 	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)

@davidradl
Copy link
Contributor

Reviewed by Chi on 09/01/2025 Go back to the submitter with review comments.

@lihaosky
Copy link
Contributor Author

@snuyanzin , thanks for the review. Addressed your comments

@lihaosky
Copy link
Contributor Author

@flinkbot run azure

@lihaosky
Copy link
Contributor Author

@flinkbot run azure re-run the last Azure build

@snuyanzin
Copy link
Contributor

@flinkbot run azure

@lihaosky
Copy link
Contributor Author

Looks the CI run is always posted in #25834 (comment)

twalthr
twalthr reviewed Feb 27, 2025
View reviewed changes
...ain/java/org/apache/flink/table/planner/operations/converters/SqlAlterModelSetConverter.java Outdated
Map<String, String> changeModelOptions =
OperationConverterUtils.extractProperties(sqlAlterModelSet.getOptionList());
if (changeModelOptions.isEmpty()) {
throw new ValidationException("ALTER MODEL SET does not support empty option");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above:
For tables we also check for immutable connector option. Do we want to do the same for models?

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

@snuyanzin snuyanzin snuyanzin left review comments

@twalthr twalthr twalthr left review comments

@davidradl davidradl davidradl left review comments

Assignees
No one assigned
Labels
component=<none>
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lihaosky @flinkbot @davidradl @snuyanzin @twalthr