New approach to CSV reading

[kosak]

Part of epic #1750

[devinrsmith]

There is a lot here. I've skipped a lot of the implementation details (on that front, I think a lot of the implementation exposure should change from public to non-public if applicable).

There are specific things around licensing we need to account for given the fast double parsing. It would be nice if we had a JMH project that demonstrated the measurable gain on using the fast double parsing over the jdk double parsing. If this is something we need, I'm happy to help setup. It may be a worthwhile project anyways, as I'd like to incorporate other microbenchmarks in other places as well.

I want to go over it at least one more time... I checked it out (had to fix a compile issue) to run some CSV files - it seemed to work for me, but I didn't go too deep.

[devinrsmith]

I think by implicitly multiplying by scale, we are artificially limiting ourselves to the implementation details for how engine DateTime expects to work. I wonder if it's better to pass values as is to the downstream sink?

[kosak]

I think I probably don't "get" the motivation here, so maybe this needs to be discussed further.
I don't see the CSV library as the right place for people to plug in arbitrary type converters.
If they have a column of longs and they want it transformed to something else, they should do that with Deephaven table "update" operations, not by plugging a new parser into their CSV reader. (in my opinion)

[devinrsmith]

Okay - I think that is a valid approach to take - in which case, we don't support arbitrary user supplied parsers. That's different than the logic as it exists today, but I'm happy to discuss and support removal of that assumption.

[kosak]

Support for custom parsing added.

[devinrsmith]

This looks like it will only use the parsers that are contained in PRECEDENCE, instead of sorting parsers. This seems wrong?

[kosak]

Yes, PRECEDENCE contains the universal list of all known parsers. In particular, users cannot define their own parsers. If users defining their own parsers is a "thing" we want, I'd like to understand more.

[kosak]

Support for custom parsing added.

[devinrsmith]

We likely need to move this to its own module and license as appropriate.

[kosak]

Agree

[devinrsmith]

At the very least, I'd want to make sure that DoubleParser passes https://github.com/srisatish/openjdk/blob/master/jdk/test/java/lang/Double/ParseDouble.java

[kosak]

I wouldn't think it's in scope for us to unit test someone else's library, but we can do it... and maybe we should. Dunno.

[devinrsmith]

I'm happy to create a fork of https://github.com/wrandelshofer/FastDoubleParser and have unit tests live there (and potentially contribute back) if they don't exist already. As a way to bulk up any unit tests, and have it live with the source library. (It would also be nice to publish it instead of bringing it into the codebase.)

Most of the bugs for https://github.com/wrandelshofer/FastDoubleParser/issues?q=is%3Aissue+is%3Aclosed seem to be around differences from the original fast double parser or differences from JDK parsing... from our side, I'd really want an easy switch back to JDK parsing.

[kosak]

JMH project that demonstrated the measurable gain on using the fast double parsing

I don't know how to reply to a comment so I will put it here. The major motivation for the third-party library was not necessarily faster double parsing per se, but because Java's builtin Double.parseDouble takes a String, not a CharSequence, and forcing me to make what could be literally tens or even hundreds of millions of short-lived temporary java.lang.Strings is a total dealkiller. I can easily benchmark how bad that is for performance.

[kosak]

Note: I have addressed many of the issues brought up here (thank you, reviewers) but I didn't get to everything. I thought you might want an updated view of what I have so far.

[jcferretti]

Can you please do a test run from IntelliJ including code coverage, so that we get an idea
of the coverage for the new files?

[kosak]

Can you please do a test run from IntelliJ including code coverage, so that we get an idea of the coverage for the new files?

OK, great idea. I've got some unreachable code so that (at least) means I need some more tests. Thank you.

[kosak]

OK thanks to this I've added a ton more tests. Coverage is "pretty high" now.

[kosak]

I think I have addressed everything (or maybe almost) everything that was asked for. If items are still open they are waiting for you to either comment on them or mark them closed.

[jcferretti]

We discussed over slack this example from python:

    if inference is None:
        inference = INFERENCE_STANDARD

    csv_specs_builder = _JCsvSpecs.builder()

    # build the head spec
    table_header = _build_header(header)
    if table_header:
        csv_specs_builder.header(table_header)

    csv_specs = (csv_specs_builder.inference(inference)
                 .hasHeaderRow(not headless)
                 .delimiter(ord(delimiter))
                 .quote(ord(quote))
                 .ignoreSurroundingSpaces(ignore_surrounding_spaces)
                 .trim(trim)
                 .charset(_JCharset.forName(charset))
                 .build())

You mentioned there is no mapping anymore for the .charset method (since we are UTF-8 everywhere all the time), and we agreed we should remove it. I am creating this comment to ensure we don't forget.

[kosak]

You mentioned there is no mapping anymore for the .charset method (since we are UTF-8 everywhere all the time), and we agreed we should remove it.

Removed.

[devinrsmith]

I don't have any bones to pick. Once Cristian happy, let's merge. And then soon we can start work on externalizing it.

[jcferretti]

Fixes #1571
Fixes #1561