[GitHub] flink issue #2303: [FLINK-4248] [core] [table] CsvTableSource does not suppo...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/2303 I still think that methods should be named after what they do (and there is nothing numeric about their behavior) and not in which context they are supposed to be called. The error messages are numeric due to the original context of the code which has been moved into separate methods. That context is no longer present in these methods. Anyway, I don't want to start a bikeshedding discussion about the names of internal utility methods and stop at this point ;-) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2303: [FLINK-4248] [core] [table] CsvTableSource does not suppo...
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2303 @fhueske I think we should keep the methods as they currently are. I renamed them to "nextNumericString"/"nextNumericStringEndPos" and added a explanation what a numeric string is. You are right they don't do much numeric at the moment but only the numeric classes will use it. Furthermore, the error states and exceptions are numeric. E.g. if we don't do the whitespace checking in `nextNumericString` we also need to return the position instead of the string. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2303: [FLINK-4248] [core] [table] CsvTableSource does not suppo...
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2303 @fhueske thanks for reviewing my code. I addressed your feedback. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2303: [FLINK-4248] [core] [table] CsvTableSource does not suppo...
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2303 @gallenvara I thought the same. But I'm not sure if it is worth it. You could save ~25 lines of code per class maybe and if we want to make the parsing more efficient (do not use the `valueOf` methods, but own implementation) the super class becomes obsolete anyway. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2303: [FLINK-4248] [core] [table] CsvTableSource does not suppo...
Github user gallenvara commented on the issue: https://github.com/apache/flink/pull/2303 Hi @twalthr , the logical of parserFields method in Date/Time/TimeStamp (also in the lastest PR for BigDecimal/BigInteger) is same, is it better to refactor by creating super class for them to reduce the duplicate codes? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
