Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
1996fanrui commented on PR #117: URL: https://github.com/apache/flink-connector-jdbc/pull/117#issuecomment-2141086375 > Maybe we should revert this commit and re-commit it in the next second or third version Sounds make sense to me. The JdbcSource is introduced in the current version, it may be not stable. So it's better to consider it as the default Jdbc connector for SQL job after community think it's stable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal commented on PR #117: URL: https://github.com/apache/flink-connector-jdbc/pull/117#issuecomment-2139538600 Hi, @1996fanrui . Kafka introduced KafkaSource API in the Flink 1.12 branch https://issues.apache.org/jira/browse/FLINK-18323 Afterwards, in Flink version 1.14, KafkaDynamicTableSource was implemented based on KafkaSource https://issues.apache.org/jira/browse/FLINK-22914 So, Maybe we should revert this commit. Please let me know what's your opinion. CC @eskabetxe -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
1996fanrui merged PR #117: URL: https://github.com/apache/flink-connector-jdbc/pull/117 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal commented on code in PR #117:
URL:
https://github.com/apache/flink-connector-jdbc/pull/117#discussion_r1593792023
##
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactoryTest.java:
##
@@ -144,7 +146,8 @@ void testJdbcReadProperties() {
readOptions,
LookupOptions.MAX_RETRIES.defaultValue(),
null,
-SCHEMA.toPhysicalRowDataType());
+SCHEMA.toPhysicalRowDataType(),
+FactoryMocks.IDENTIFIER.asSummaryString());
Review Comment:
@eskabetxe updated! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
eskabetxe commented on code in PR #117:
URL:
https://github.com/apache/flink-connector-jdbc/pull/117#discussion_r1593526480
##
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactoryTest.java:
##
@@ -144,7 +146,8 @@ void testJdbcReadProperties() {
readOptions,
LookupOptions.MAX_RETRIES.defaultValue(),
null,
-SCHEMA.toPhysicalRowDataType());
+SCHEMA.toPhysicalRowDataType(),
+FactoryMocks.IDENTIFIER.asSummaryString());
Review Comment:
If is overwrite I would say yes, we should drop the attribute, as at the end
is not used..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal commented on code in PR #117:
URL:
https://github.com/apache/flink-connector-jdbc/pull/117#discussion_r1593374629
##
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactoryTest.java:
##
@@ -144,7 +146,8 @@ void testJdbcReadProperties() {
readOptions,
LookupOptions.MAX_RETRIES.defaultValue(),
null,
-SCHEMA.toPhysicalRowDataType());
+SCHEMA.toPhysicalRowDataType(),
+FactoryMocks.IDENTIFIER.asSummaryString());
Review Comment:
I added the corresponding cases for testing.
As shown in the screenshot pics, The name is overwrite by the translation
mechanism.
Maybe we should drop the `tableIdentifier` attribute at present ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal commented on code in PR #117:
URL:
https://github.com/apache/flink-connector-jdbc/pull/117#discussion_r1593372606
##
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableSource.java:
##
@@ -179,13 +205,34 @@ public ScanRuntimeProvider
getScanRuntimeProvider(ScanContext runtimeProviderCon
LOG.debug("Query generated for JDBC scan: " + query);
-builder.setQuery(query);
+builder.setSql(query);
final RowType rowType = (RowType) physicalRowDataType.getLogicalType();
-builder.setRowConverter(dialect.getRowConverter(rowType));
-builder.setRowDataTypeInfo(
-
runtimeProviderContext.createTypeInformation(physicalRowDataType));
+builder.setResultExtractor(new
RowDataResultExtractor(dialect.getRowConverter(rowType)));
+
builder.setTypeInformation(scanContext.createTypeInformation(physicalRowDataType));
+options.getProperties()
+.forEach(
+(key, value) ->
+builder.setConnectionProperty(key.toString(),
value.toString()));
+JdbcSource source = builder.build();
+return new DataStreamScanProvider() {
Review Comment:
updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
eskabetxe commented on code in PR #117:
URL:
https://github.com/apache/flink-connector-jdbc/pull/117#discussion_r1592285942
##
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactoryTest.java:
##
@@ -144,7 +146,8 @@ void testJdbcReadProperties() {
readOptions,
LookupOptions.MAX_RETRIES.defaultValue(),
null,
-SCHEMA.toPhysicalRowDataType());
+SCHEMA.toPhysicalRowDataType(),
+FactoryMocks.IDENTIFIER.asSummaryString());
Review Comment:
could we test in some way the source naming?
##
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableSource.java:
##
@@ -179,13 +205,34 @@ public ScanRuntimeProvider
getScanRuntimeProvider(ScanContext runtimeProviderCon
LOG.debug("Query generated for JDBC scan: " + query);
-builder.setQuery(query);
+builder.setSql(query);
final RowType rowType = (RowType) physicalRowDataType.getLogicalType();
-builder.setRowConverter(dialect.getRowConverter(rowType));
-builder.setRowDataTypeInfo(
-
runtimeProviderContext.createTypeInformation(physicalRowDataType));
+builder.setResultExtractor(new
RowDataResultExtractor(dialect.getRowConverter(rowType)));
+
builder.setTypeInformation(scanContext.createTypeInformation(physicalRowDataType));
+options.getProperties()
+.forEach(
+(key, value) ->
+builder.setConnectionProperty(key.toString(),
value.toString()));
+JdbcSource source = builder.build();
+return new DataStreamScanProvider() {
Review Comment:
should we create a static class (inside this file) for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal closed pull request #117: [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source URL: https://github.com/apache/flink-connector-jdbc/pull/117 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal commented on PR #117: URL: https://github.com/apache/flink-connector-jdbc/pull/117#issuecomment-2077036231 HI, @eskabetxe @caicancai Could you help have a review if you had the free time ? thx a lot. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal commented on PR #117: URL: https://github.com/apache/flink-connector-jdbc/pull/117#issuecomment-2074135667 blocked by https://github.com/apache/flink-connector-jdbc/pull/116 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal closed pull request #117: [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source URL: https://github.com/apache/flink-connector-jdbc/pull/117 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source [flink-connector-jdbc]
RocMarshal opened a new pull request, #117: URL: https://github.com/apache/flink-connector-jdbc/pull/117 - Support the implementation of dynamic source tables based on the new source -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
