-
Notifications
You must be signed in to change notification settings - Fork 169
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-34467] add lineage integration for jdbc connector #149
base: main
Are you sure you want to change the base?
Conversation
f1f5587
to
d468795
Compare
d468795
to
231ad7d
Compare
231ad7d
to
17c6ad8
Compare
@HuangZhenQiu I notice your comment "The PR is not ready for review.", I wonder if you could turn this PR into a draft until it is ready for review - thanks. |
17c6ad8
to
4719928
Compare
4719928
to
2d0ba42
Compare
|
||
/** Default implementation of {@link TypeDatasetFacet}. */ | ||
@PublicEvolving | ||
public class DefaultTypeDatasetFacet implements TypeDatasetFacet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point this should be moved to flink-core
but I understand why this hasn't been done prior to this PR.
public class Db2LocationExtractor implements JdbcLocationExtractor { | ||
|
||
private JdbcLocationExtractor delegate() { | ||
return new OverrideJdbcLocationExtractor("db2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be better to have delegate as class member instead of creating it each call?
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.flink.connector.jdbc.cratedb.database.lineage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to implement those classes within openlineage-sql-java
package?
It's kind of a namespace convention thing, and implementing this a generic way, which is reusable across the integrations, can be a better solution. what do you think?
return nameOf(simpleJdbcQueryStatement.query()); | ||
} | ||
|
||
public static Optional<String> nameOf(String query) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify what name is this. Dataset name, job name?
2d0ba42
to
032eab5
Compare
@HuangZhenQiu It would be worth changing this text in the PR, as I think the PR is ready for review. |
Add native lineage support for JDBC connector. It is an ongoing effort. The PR is not ready for review.