[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15333299#comment-15333299
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2007
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15332545#comment-15332545
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2007
Merging
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15313872#comment-15313872
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2007
I'm not sure; in any case it should not be removed as part of this PR.
You can open a separate JIRA or ask on the mailing list.
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15313864#comment-15313864
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user fpompermaier commented on the issue:
https://github.com/apache/flink/pull/2007
Now it should be ok, according to your suggestions. I misunderstood what
@StephanEwen was trying to say, thanks @zentol for the clarification!
Just another thing: the method GenericCsvInputFormat.checkAndCoSort() is
never used in the code. Do you want to keep it?
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15310646#comment-15310646
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2007
then make parseField protected so users MUST call resetErrorStateAndParse.
there, problem solved.
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15310486#comment-15310486
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user fpompermaier commented on the issue:
https://github.com/apache/flink/pull/2007
I think that leaving the responsibility of calling
resetErrorStateAndParse() to "consumers" of FieldParser is really a bad idea.
Isn't safer to force its call using the strategy adopted in this PR (i.e. keep
the real implementation in parseFieldImpl and use parseField to call
resetErrorStateAndParse before it)?
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15310383#comment-15310383
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/2007
i believe what Stephan meant was the following:
* keep the abstract parseField method in FieldParser
* add a new method to FieldParser called resetErrorStateAndParse, which
calls resetParserState and parseField
This would require reverting all changes you made to specific parsers (like
LongValueParser), and modifiying each call to parseField from outside the
parser, like the GenericCsvInputFormat.
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15310227#comment-15310227
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user fpompermaier commented on the pull request:
https://github.com/apache/flink/pull/2007
How should I proceed here?
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15302107#comment-15302107
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user fpompermaier commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221880071
I don't understand :(
Assuming that `reset` could be renamed as `resetErrorStateAndParse`, the
other 2 suggestions cannot be applied to my current implementation: if I want
to rename `parseFieldImpl` to `parseField` I should overload somehow the method
otherwise that's not possible because there's already an abstract method called
parseField with the same sign..
Wrt leaving the responsibility to reset the state of the parsers to the
classes calling the parseField is quite dangerous IMHo (as you said in
"GenericCsvInputCormat would call the resetErrorStateAndParse").
Am I misunderstanding something?
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15302081#comment-15302081
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221876052
I think I meant it a bit differently than I guess you understood it.
It is basically the exact same implementation you have now, only with
different method names.
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15302031#comment-15302031
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user fpompermaier commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221862570
Hi Stephan, thanks for reviewing this PR.
These are my reasons to not apply your suggestions to my PR, and I'd like
to hear your opinion (anyone interested by this discussion is also welcome)
before reissuing the PR with such modifications:
- rename `reset` to `resetErrorStateAndParse`: for almost all parsers this
doesn't do anything more than those 2 operations but what if for a further
parserImpl you need to do anything else before parsing?
- resetting the error state is something that must always be done, by all
parser implementations. The `parseFieldImpl` is a way to force this thing,
otherwise you should rely on classes using such parser to remember this thing
(as you suggested with "GenericCsvInputCormat would call the
resetErrorStateAndParse") or ensure that every parser call it at the very
beginning of the `parseField()`.
I'm really concerned about this. In this way, my PR doesn't break the
current APIs
Does it makes sense?
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15302011#comment-15302011
]
ASF GitHub Bot commented on FLINK-3908:
---
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221858427
Thanks for addressing this bug!
How about slightly changing the names of the methods:
- The method that resets the error state and parses would be called
`resetErrorStateAndParse(...)`. That would make it clear what happens.
- The `parseFieldImpl(...)` could be kept as `parseField(...)`
- The `GenericCsvInputCormat` would call the
`resetErrorStateAndParse(...)` method.
Other than that, solution looks good!
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15290824#comment-15290824
]
Flavio Pompermaier commented on FLINK-3908:
---
In order to make FLINK-3901 work properly this issue should be merged
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (FLINK-3908) FieldParsers error state is not reset correctly to NONE
[
https://issues.apache.org/jira/browse/FLINK-3908?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15290821#comment-15290821
]
Flavio Pompermaier commented on FLINK-3908:
---
In order to make FLINK-3908 work properly this issue should be merged
> FieldParsers error state is not reset correctly to NONE
> ---
>
> Key: FLINK-3908
> URL: https://issues.apache.org/jira/browse/FLINK-3908
> Project: Flink
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.0.2
>Reporter: Flavio Pompermaier
>Assignee: Flavio Pompermaier
> Labels: parser
>
> If during the parse of a csv there's a parse error (for example when in a
> integer column there are non-int values) the errorState is not reset
> correctly in the next parseField call. A simple fix would be to add as a
> first statement of the {{parseField()}} function a call to
> {{setErrorState(ParseErrorState.NONE)}} but it is something that should be
> handled better (by default) for every subclass of {{FieldParser}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
