You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "davidradl (via GitHub)" <gi...@apache.org> on 2025/12/04 17:20:49 UTC
[PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
davidradl opened a new pull request, #10:
URL: https://github.com/apache/flink-connector-http/pull/10
(no comment)
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "ferenc-csaky (via GitHub)" <gi...@apache.org>.
ferenc-csaky merged PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#discussion_r2596540948
##########
flink-connector-http/src/test/java/org/apache/flink/connector/http/table/lookup/HttpLookupTableSourceITCaseTest.java:
##########
@@ -1123,6 +1127,35 @@ private void assertEnrichedRowsNoDataBadStatus(Collection<Row> collectedRows) {
});
}
+ private void assertEnrichedRowsNoDataGoodStatus(Collection<Row> collectedRows) {
+
+ final int rowArity = 10;
+ // validate every row and its column.
+
+ assertAll(
Review Comment:
@RocMarshal Good idea. I have raised [https://issues.apache.org/jira/browse/FLINK-38789](https://issues.apache.org/jira/browse/FLINK-38789) to track 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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#discussion_r2593047048
##########
docs/content.zh/docs/connectors/table/http.md:
##########
@@ -241,6 +241,25 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
+In order to use custom format, user has to specify option `'lookup-request.format' = 'customFormatName'`, where `customFormatName` is the identifier of custom format factory.
Review Comment:
```suggestion
In order to use custom format, users have to specify the option `'lookup-request.format' = 'customFormatName'`, where `customFormatName` is the identifier of custom format factory.
```
##########
flink-connector-http/src/test/java/org/apache/flink/connector/http/table/lookup/HttpLookupTableSourceITCaseTest.java:
##########
@@ -1123,6 +1127,35 @@ private void assertEnrichedRowsNoDataBadStatus(Collection<Row> collectedRows) {
});
}
+ private void assertEnrichedRowsNoDataGoodStatus(Collection<Row> collectedRows) {
+
+ final int rowArity = 10;
+ // validate every row and its column.
+
+ assertAll(
Review Comment:
I noticed that the API style in the assertion sections of the Flink HTTP test cases is quite arbitrary, with a mix of Jupiter Assertions and AssertJ Assertions being used.
I’d like to briefly share two small suggestions:
- A: Wouldn’t it be better to align with the assertion style used in the Flink repository? That is, to use only AssertJ-related assertion APIs to implement assertion statements.
- B: Building on A, we could handle the existing assertion statements as follows:
- a) There’s no need to rush and change them to the AssertJ style, as this is a historical issue and not changing it won’t affect the assertion functionality. It would be a good approach to restart the API migration for this part when our contributors have enough bandwidth to focus on it.
- b) For newly introduced test case code, we expect to use AssertJ assertion APIs to complete the assertion statements.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#issuecomment-3613381316
@ferenc-csaky Please could you have a look at this one.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#discussion_r2593123531
##########
flink-connector-http/src/test/java/org/apache/flink/connector/http/table/lookup/HttpLookupTableSourceITCaseTest.java:
##########
@@ -1123,6 +1127,35 @@ private void assertEnrichedRowsNoDataBadStatus(Collection<Row> collectedRows) {
});
}
+ private void assertEnrichedRowsNoDataGoodStatus(Collection<Row> collectedRows) {
+
+ final int rowArity = 10;
+ // validate every row and its column.
+
+ assertAll(
Review Comment:
Thanks @davidradl for the contribution.
I noticed that the API style in the assertion sections of the Flink HTTP test cases is quite arbitrary, with a mix of Jupiter Assertions and AssertJ Assertions being used.
I’d like to briefly share two small suggestions:
- A: Could we align with the assertion style used in the Flink repository? That is, to use only AssertJ-related assertion APIs to implement assertion statements.
- B: Building on A, we could handle the existing assertion statements as follows:
- a) There’s no need to rush and change them to the AssertJ style, as this is a historical issue and not changing it won’t affect the assertion functionality. It would be a good approach to restart the API migration for this part when our contributors have enough bandwidth to focus on it.
- b) For newly introduced test case code, we expect to use AssertJ assertion APIs to complete the assertion statements.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "ferenc-csaky (via GitHub)" <gi...@apache.org>.
ferenc-csaky commented on code in PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#discussion_r2607242594
##########
docs/content.zh/docs/connectors/table/http.md:
##########
@@ -241,6 +241,27 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
Review Comment:
http -> HTTP
##########
docs/content.zh/docs/connectors/table/http.md:
##########
@@ -241,6 +241,27 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
+In order to use a custom format, users have to specify the option `'lookup-request.format' = '{customFormatName}'`, where `{customFormatName}` is the identifier of the custom format factory.
+Additionally, it is possible to pass custom query format options from table's DDL.
+This can be done by: `'lookup-request.format.{customFormatName}.{customFormatProperty}' = '{propertyValue}'`, where {customFormatProperty} is the name of a custom
+property and {propertyValue} is the property value.
+For example:
+`'lookup-request.format.myCustomFormatName.foo' = 'baa'`.
+
+With the default configuration, flink-Json format is used for `GenericGetQueryCreator`; all options defined in [json-format](https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/table/formats/json/) can be passed through the table DDL.
+For example:
+`'lookup-request.format.json.fail-on-missing-field' = 'true'`.
+
+#### For http responses
Review Comment:
http -> HTTP
##########
docs/content/docs/connectors/table/http.md:
##########
@@ -240,6 +241,27 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
Review Comment:
http -> HTTP
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/HttpLookupTableSourceFactory.java:
##########
@@ -97,18 +97,25 @@ public DynamicTableSource createDynamicTableSource(Context dynamicTableContext)
FactoryUtil.createTableFactoryHelper(this, dynamicTableContext);
ReadableConfig readable = helper.getOptions();
+
+ // Discover and validate the decoding format first - this validates format-specific options
Review Comment:
IMO this comment is redundant, we have 6 more 5 lines later that are a lot more specific. Comments has to be maintained just like code, so it should have a reason to be. :)
##########
docs/content/docs/connectors/table/http.md:
##########
@@ -240,6 +241,27 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
+In order to use a custom format, users have to specify the option `'lookup-request.format' = '{customFormatName}'`, where `{customFormatName}` is the identifier of the custom format factory.
+Additionally, it is possible to pass custom query format options from table's DDL.
+This can be done by: `'lookup-request.format.{customFormatName}.{customFormatProperty}' = '{propertyValue}'`, where {customFormatProperty} is the name of a custom
+property and {propertyValue} is the property value.
+For example:
+`'lookup-request.format.myCustomFormatName.foo' = 'baa'`.
+
+With the default configuration, flink-Json format is used for `GenericGetQueryCreator`; all options defined in [json-format](https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/table/formats/json/) can be passed through the table DDL.
+For example:
+`'lookup-request.format.json.fail-on-missing-field' = 'true'`.
+
+#### For http responses
Review Comment:
http -> HTTP
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/HttpTableLookupFunction.java:
##########
@@ -120,11 +120,16 @@ public Collection<RowData> lookup(RowData keyRow) {
}
}
// if we did not get the physical arity from the http response physical row then get it from
- // the producedDataType, which is set when we have metadata
- if (physicalArity == -1 && producedDataType != null) {
- List<LogicalType> childrenLogicalTypes =
- producedDataType.getLogicalType().getChildren();
- physicalArity = childrenLogicalTypes.size() - metadataArity;
+ // the producedDataType. which is set when we have metadata or when there's no data
+ if (physicalArity == -1) {
+ if (producedDataType != null) {
Review Comment:
Lets invert this check, so we can return empty first, and then spare the extra indentation level for the code that updates `physicalArity`.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#discussion_r2606125425
##########
docs/content/docs/connectors/table/http.md:
##########
@@ -240,6 +240,25 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
+In order to use custom format, user has to specify option `'lookup-request.format' = 'customFormatName'`, where `customFormatName` is the identifier of custom format factory.
Review Comment:
I have made some amendments to improve the English, prompted by your suggestions. @RocMarshal let me know what you think.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FLINK-38781 pass format options to response using json format [flink-connector-http]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #10:
URL: https://github.com/apache/flink-connector-http/pull/10#discussion_r2596857692
##########
docs/content/docs/connectors/table/http.md:
##########
@@ -240,6 +240,25 @@ POST, PUT and GET operations. This query creator allows you to issue json reques
your own custom http connector. The mappings from columns to the json request are supplied in the query creator configuration
parameters `http.request.query-param-fields`, `http.request.body-fields` and `http.request.url-map`.
+### Format considerations
+
+#### For http requests
+In order to use custom format, user has to specify option `'lookup-request.format' = 'customFormatName'`, where `customFormatName` is the identifier of custom format factory.
Review Comment:
Hi, @davidradl Thx for the quickly response.
Sorry for forgetting adding comment here in the previous round, too.
So, ditto with the previous comments.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org