You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/07/05 20:31:00 UTC
[GitHub] [flink] BryceCicada opened a new pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
BryceCicada opened a new pull request #12824:
URL: https://github.com/apache/flink/pull/12824
## What is the purpose of the change
CsvTableSource in Python API is missing a number of constructor params compared to the Java API.
## Brief change log
- *Added additional parameters to CsvTableSource in Python API*
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## 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 (pyflink)
- 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/Mesos, 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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a112ae76f326f1abfa2f921dcdb0acd5f63ebf25 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245)
* 6f8b0d3951c82e96839ef2c02e36165793c5a098 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r450083499
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
gateway = get_gateway()
- j_field_names = utils.to_jarray(gateway.jvm.String, field_names)
- j_field_types = utils.to_jarray(gateway.jvm.TypeInformation,
- [_to_java_type(field_type)
- for field_type in field_types])
- super(CsvTableSource, self).__init__(
- gateway.jvm.CsvTableSource(source_path, j_field_names, j_field_types))
+
+ builder = gateway.jvm.CsvTableSource.builder()
+ builder.path(source_path)
+
+ for (field_name, field_type) in zip(field_names, field_types):
+ builder.field(field_name, _to_java_type(field_type))
+
+ if field_delim is not None:
+ builder.fieldDelimiter(field_delim)
+
+ if line_delim is not None:
+ builder.lineDelimiter(line_delim)
+
+ if quote_character is not None:
+ builder.quoteCharacter(quote_character)
Review comment:
I've added the Python str length sanity check.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
}, {
"hash" : "e969d039e6d563986f609dedd1f93b7485242649",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e969d039e6d563986f609dedd1f93b7485242649",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6f8b0d3951c82e96839ef2c02e36165793c5a098 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261)
* e969d039e6d563986f609dedd1f93b7485242649 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a112ae76f326f1abfa2f921dcdb0acd5f63ebf25 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r450083027
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
Review comment:
Thanks. I've removed it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653935647
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit a112ae76f326f1abfa2f921dcdb0acd5f63ebf25 (Sun Jul 05 20:32:42 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
* **This pull request references an unassigned [Jira ticket](https://issues.apache.org/jira/browse/FLINK-18488).** According to the [code contribution guide](https://flink.apache.org/contributing/contribute-code.html), tickets need to be assigned before starting with the implementation work.
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
}, {
"hash" : "e969d039e6d563986f609dedd1f93b7485242649",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264",
"triggerID" : "e969d039e6d563986f609dedd1f93b7485242649",
"triggerType" : "PUSH"
}, {
"hash" : "813832c9fee2d70bde2d3d4d8839ff66bc41430b",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4278",
"triggerID" : "813832c9fee2d70bde2d3d4d8839ff66bc41430b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e969d039e6d563986f609dedd1f93b7485242649 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264)
* 813832c9fee2d70bde2d3d4d8839ff66bc41430b Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4278)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r450066743
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
gateway = get_gateway()
- j_field_names = utils.to_jarray(gateway.jvm.String, field_names)
- j_field_types = utils.to_jarray(gateway.jvm.TypeInformation,
- [_to_java_type(field_type)
- for field_type in field_types])
- super(CsvTableSource, self).__init__(
- gateway.jvm.CsvTableSource(source_path, j_field_names, j_field_types))
+
+ builder = gateway.jvm.CsvTableSource.builder()
+ builder.path(source_path)
+
+ for (field_name, field_type) in zip(field_names, field_types):
+ builder.field(field_name, _to_java_type(field_type))
+
+ if field_delim is not None:
+ builder.fieldDelimiter(field_delim)
+
+ if line_delim is not None:
+ builder.lineDelimiter(line_delim)
+
+ if quote_character is not None:
+ builder.quoteCharacter(quote_character)
Review comment:
Interestingly, it works ok. Py4J appears to be selecting the first character of the Python string as the Java Character.
But yeah, there is some surprise in this so it would make sense for me to add some validation of `len(quote_character) == 1`.
UPDATE: Here's the Py4J code. https://github.com/bartdag/py4j/blob/4152353ac142a7c6d177e0d8f5d420d92c846a30/py4j-java/src/main/java/py4j/reflection/TypeConverter.java#L85
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
}, {
"hash" : "e969d039e6d563986f609dedd1f93b7485242649",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264",
"triggerID" : "e969d039e6d563986f609dedd1f93b7485242649",
"triggerType" : "PUSH"
}, {
"hash" : "813832c9fee2d70bde2d3d4d8839ff66bc41430b",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "813832c9fee2d70bde2d3d4d8839ff66bc41430b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e969d039e6d563986f609dedd1f93b7485242649 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264)
* 813832c9fee2d70bde2d3d4d8839ff66bc41430b UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dianfu commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
dianfu commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r449959865
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
Review comment:
This line doesn't apply any more. What about just remove it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-654121466
@dianfu Thanks.
> Should we also support emptyColumnAsNull?
Yeah... from the Java side, I think some small mismatch between the CsvTableSource constructor and builder has crept in:
- The builder allows emptyColumnAsNull, but the constructors do not.
- There's one constructor that allows selectedFields, but the builder does not.
If I add emptyColumnAsNull to Python constructor, it'll make the Python constructor match none of the Java constructors. My initial thoughts were that it would be a little jarring. On the other-hand, without it we're depriving python users of the feature. So yeah, I agree... we should add it here.
I'll raise another Jira ticket describing the Java constructor/builder thing and request it's assigned to me. Once that's implemented, the Python and Java constructors should match.
> Besides, there are checkstyle issues which should be addressed.
Thanks, I've just seen how to run these and I see what you mean. Will fix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r450066743
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
gateway = get_gateway()
- j_field_names = utils.to_jarray(gateway.jvm.String, field_names)
- j_field_types = utils.to_jarray(gateway.jvm.TypeInformation,
- [_to_java_type(field_type)
- for field_type in field_types])
- super(CsvTableSource, self).__init__(
- gateway.jvm.CsvTableSource(source_path, j_field_names, j_field_types))
+
+ builder = gateway.jvm.CsvTableSource.builder()
+ builder.path(source_path)
+
+ for (field_name, field_type) in zip(field_names, field_types):
+ builder.field(field_name, _to_java_type(field_type))
+
+ if field_delim is not None:
+ builder.fieldDelimiter(field_delim)
+
+ if line_delim is not None:
+ builder.lineDelimiter(line_delim)
+
+ if quote_character is not None:
+ builder.quoteCharacter(quote_character)
Review comment:
Interestingly, it works ok. Py4J appears to be selecting the first character of the Python string as the Java Character.
But yeah, there is some surprise in this so it would make sense for me to add some validation of `len(quote_character) == 1`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dianfu commented on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
dianfu commented on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-654112165
> @BryceCicada Good catch! Thanks a lot for the contribution. Just left a few minor comments. Should we also support emptyColumnAsNull? Besides, there are checkstyle issues which should be addressed.
@BryceCicada Thanks for the update. There are check style issues in the first commit which failed the tests. It seems that it's not addressed in the latest PR. I guess you missed this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada commented on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada commented on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-654121466
@dianfu Thanks.
> Should we also support emptyColumnAsNull?
Yeah... from the Java side, I think some small mismatch between the CsvTableSource constructor and builder has crept in:
- The builder allows emptyColumnAsNull, but the constructors do not.
- There's one constructor that allows selectedFields, but the builder does not.
If I add this, it'll make the Python constructor match none of the Java constructors. My initial thoughts were that it would be a little jarring. On the other-hand, without it we're depriving python users of the feature. So yeah, I agree... we should add it here.
I'll raise another Jira ticket describing the Java constructor/builder thing and request it's assigned to me. Once that's implemented, the Python and Java constructors should match.
> Besides, there are checkstyle issues which should be addressed.
Thanks, I've just seen how to run these and I see what you mean. Will fix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dianfu commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
dianfu commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r450078253
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
gateway = get_gateway()
- j_field_names = utils.to_jarray(gateway.jvm.String, field_names)
- j_field_types = utils.to_jarray(gateway.jvm.TypeInformation,
- [_to_java_type(field_type)
- for field_type in field_types])
- super(CsvTableSource, self).__init__(
- gateway.jvm.CsvTableSource(source_path, j_field_names, j_field_types))
+
+ builder = gateway.jvm.CsvTableSource.builder()
+ builder.path(source_path)
+
+ for (field_name, field_type) in zip(field_names, field_types):
+ builder.field(field_name, _to_java_type(field_type))
+
+ if field_delim is not None:
+ builder.fieldDelimiter(field_delim)
+
+ if line_delim is not None:
+ builder.lineDelimiter(line_delim)
+
+ if quote_character is not None:
+ builder.quoteCharacter(quote_character)
Review comment:
Thanks for the link. You're right that Py4J could handle this correctly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dianfu closed pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
dianfu closed pull request #12824:
URL: https://github.com/apache/flink/pull/12824
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
}, {
"hash" : "e969d039e6d563986f609dedd1f93b7485242649",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264",
"triggerID" : "e969d039e6d563986f609dedd1f93b7485242649",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e969d039e6d563986f609dedd1f93b7485242649 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a112ae76f326f1abfa2f921dcdb0acd5f63ebf25 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a112ae76f326f1abfa2f921dcdb0acd5f63ebf25 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
}, {
"hash" : "e969d039e6d563986f609dedd1f93b7485242649",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264",
"triggerID" : "e969d039e6d563986f609dedd1f93b7485242649",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6f8b0d3951c82e96839ef2c02e36165793c5a098 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4261)
* e969d039e6d563986f609dedd1f93b7485242649 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4264)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] BryceCicada commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
BryceCicada commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r450066743
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
gateway = get_gateway()
- j_field_names = utils.to_jarray(gateway.jvm.String, field_names)
- j_field_types = utils.to_jarray(gateway.jvm.TypeInformation,
- [_to_java_type(field_type)
- for field_type in field_types])
- super(CsvTableSource, self).__init__(
- gateway.jvm.CsvTableSource(source_path, j_field_names, j_field_types))
+
+ builder = gateway.jvm.CsvTableSource.builder()
+ builder.path(source_path)
+
+ for (field_name, field_type) in zip(field_names, field_types):
+ builder.field(field_name, _to_java_type(field_type))
+
+ if field_delim is not None:
+ builder.fieldDelimiter(field_delim)
+
+ if line_delim is not None:
+ builder.lineDelimiter(line_delim)
+
+ if quote_character is not None:
+ builder.quoteCharacter(quote_character)
Review comment:
Interestingly, it works ok. Py4J appears to be selecting the first character of the Python string as the Java Character.
But yeah, there is some surprise in this so it would make sense for me to add some validation of `len(quote_character) == 1`. It would catch cases where a user expects a quote to be '//', but the parser would silently ignore anything prefixed with just a single '/'
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dianfu commented on a change in pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
dianfu commented on a change in pull request #12824:
URL: https://github.com/apache/flink/pull/12824#discussion_r449958503
##########
File path: flink-python/pyflink/table/sources.py
##########
@@ -38,16 +38,62 @@ class CsvTableSource(TableSource):
(logically) unlimited number of fields.
:param source_path: The path to the CSV file.
+ :type source_path: str
:param field_names: The names of the table fields.
+ :type field_names: collections.Iterable[str]
:param field_types: The types of the table fields.
+ :type field_types: collections.Iterable[str]
+ :param field_delim: The field delimiter, "," by default.
+ :type field_delim: str, optional
+ :param line_delim: The row delimiter, "\\n" by default.
+ :type line_delim: str, optional
+ :param quote_character: An optional quote character for String values, null by default.
+ :type quote_character: str, optional
+ :param ignore_first_line: Flag to ignore the first line, false by default.
+ :type ignore_first_line: bool, optional
+ :param ignore_comments: An optional prefix to indicate comments, null by default.
+ :type ignore_comments: str, optional
+ :param lenient: Flag to skip records with parse error instead to fail, false by default.
+ :type lenient: bool, optional
"""
- def __init__(self, source_path, field_names, field_types):
+ def __init__(
+ self,
+ source_path,
+ field_names,
+ field_types,
+ field_delim=None,
+ line_delim=None,
+ quote_character=None,
+ ignore_first_line=None,
+ ignore_comments=None,
+ lenient=None,
+ ):
# type: (str, list[str], list[DataType]) -> None
gateway = get_gateway()
- j_field_names = utils.to_jarray(gateway.jvm.String, field_names)
- j_field_types = utils.to_jarray(gateway.jvm.TypeInformation,
- [_to_java_type(field_type)
- for field_type in field_types])
- super(CsvTableSource, self).__init__(
- gateway.jvm.CsvTableSource(source_path, j_field_names, j_field_types))
+
+ builder = gateway.jvm.CsvTableSource.builder()
+ builder.path(source_path)
+
+ for (field_name, field_type) in zip(field_names, field_types):
+ builder.field(field_name, _to_java_type(field_type))
+
+ if field_delim is not None:
+ builder.fieldDelimiter(field_delim)
+
+ if line_delim is not None:
+ builder.lineDelimiter(line_delim)
+
+ if quote_character is not None:
+ builder.quoteCharacter(quote_character)
Review comment:
I guess this doesn't work as the Java method **quoteCharacter** only accepts type of **Character**. However, **quote_character** will be converted to *String* type by Py4J.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12824: [FLINK-18488][python] Added additional params to CsvTableSource constructor
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12824:
URL: https://github.com/apache/flink/pull/12824#issuecomment-653937012
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245",
"triggerID" : "a112ae76f326f1abfa2f921dcdb0acd5f63ebf25",
"triggerType" : "PUSH"
}, {
"hash" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "6f8b0d3951c82e96839ef2c02e36165793c5a098",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a112ae76f326f1abfa2f921dcdb0acd5f63ebf25 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=4245)
* 6f8b0d3951c82e96839ef2c02e36165793c5a098 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org