DRILL-7978: Fixed Width Format Plugin

[MFoss19]

DRILL-7978: Fixed Width Format Plugin

Description

Developing format plugin to parse fixed width files.

Fixed Width Text File Definition: https://www.oracle.com/webfolder/technetwork/data-quality/edqhelp/Content/introduction/getting_started/configuring_fixed_width_text_file_formats.htm

Documentation

Users can now create a format configuration to parse fixed width files.

Testing

Unit tests added. More to come.

[cgivre]

@MFoss19
Thank you very much for this contribution to Drill and nice work! I left some review comments. General comment would be to please remove any extraneous commented out code and Drill does have an automated style sheet which you can install for Intellij or Eclipse.

Also, to pass the CI and merge this, all the files will need to have the Apache license in them at the beginning of the file. The only exception would be the fwf test files. For that, you're going to have to add an exclusion to the RAT check in the root pom.xml file.

[cgivre]

Please add tests for:

• Serialization/Deserialization
• Compressed file
• Various invalid schemata. For instance, what happens if you don't have fields defined in the config and try to query the data?

[cgivre]

I think this actually might be blocksplittable.

[paul-rogers]

@MFoss19, thanks much for the contribution. This should be very helpful for folks.

My comments are about some of the nuances of creating a storage plugin. I reference a couple of Drill internals. Let me know if you want more details about these.

[paul-rogers]

Since configs are created by hand, having good defaults is helpful. Perhaps:

• name: required; throw an exception if blank, or if the stripped name is not a valid SQL symbol.
• type: default to VARCHAR
• dateTimeFormat: null is allowed, so no default.
• index: required, must be >= 0.
• width: either required, or can be optional. If provided must be > 0. (See below.)

For this plugin, we also have to check the set of fields.

• No two fields can have the same name.
• Should fields be allowed to overlap?

We could be clever. Scan all fields and sort into ascending order. If a field omits the width, just compute it from the index of this and the next field.

[paul-rogers]

Does it work to use the MinorType here? Is that type set up for Jackson serialization? I don't know the answer, just noting we should double-check to ensure it works OK.

[MFoss19]

This was tested configuring the plugin within the Drill UI (via manual test). I will also add an automated unit test for parsing the Json. To answer your question, yes it works to use MinorType here. Jackson can always read in Java enums, MinorType was generated as part of our protobuf and for this type of data they generate proper Java enums.

[paul-rogers]

I'm pretty sure fix-width files are splittable. If every records resides on a single line, they the file is spittable if we add code that, if the start offset !=0, scan to the next newline. And, on read, read rows until the file position is greater than the block end. See the text file (CSV) plugin for details (though, don't follow its implementation as that implementation is rather unique to that one use case.)

[paul-rogers]

This uses "EVF V1". Your plugin provides schema information, and is thus a perfect fit for "EVF V2" which can use your schema information to set up the row set loader schema automatically for you.

[cgivre]

@paul-rogers Is there an example somewhere of how to use the rowset loader to set up the schema automatically? Is this as simple as checking to see whether the schema is provided and if so, use that?

[paul-rogers]

@cgivre, it's been a while since I looked at that stuff. As I recall, the CSV reader was converted, but it is a bit obscure. The unit test also show what can be done, IIRC.

Basically, V2 gets schema from multiple sources:

• Schema info in the plan object (if available in the future)
• Provided schema (the current interim solution to system-provided schema)
• Schema implied by the selected columns (for example, a[1] means a has to be an array.)
• Schema defined by the data source itself (as in Parquet, or in CSV where you can choose the columns array)
• Schema discovered by the traditional "schema on read"

The V2 SchemaNegotiator provides the needed methods. V2 will then come up with the final schema. V2 lets you read the entire row (columns a, b, c, say) even if the query wants only column a: V2 silently discards the unwanted columns.

[paul-rogers]

This is OK, but slow because of the switch. There is a set of field converter classes which can handle the string-to-whatever conversions. With that, there is a direct call per field (inner loop) from reading the field to convert to write into value vectors. The field-specific-type switch is done only once, at setup time.

These converters are used in the CSV reader when a schema is provided. I can dig up more examples if helpful.

[paul-rogers]

To be clearer: what we want is to minimize the per-field work. Ideally, we'd set up an array of column converters so the loop looks like:

for (int i = 0; i < config.getFields().size(); i++) {
FixedwidthFieldConfig field = config.getFields().get(i);
value = line.substring(field.getIndex() - 1, field.getIndex() + field.getWidth() - 1);
writer.scalar(i).setString(value)
}

However, the above requires the user to specify the config. For every file. On every Drill cluster. Better, if the schema is not given, infer it from the first (few) row(s). Then, set up runtime field objects:

```java
for (FieldReader fieldReader : fieldReaders) {
  fieldReader.load(line);
}

The field reader:

• Remembers the start and offset
• Pulls out the string, handling a truncated line
• Calls a cached column writer

@cgivre mentioned the idea of a column converter. There is a defined set for the common cases. The underlying mechanism sets them up for you. (V2 makes it simpler.) That way, a call to setString() directly invokes the thing that converts from string and writes the resulting value: no per-column switch needed.

[lgtm-com]

This pull request introduces 2 alerts when merging 2d17f1b into 0c9451e - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 18380ea into f4ea90c - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging a91be4c into f4ea90c - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging dc60d28 into b6da35e - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 05ae3f1 into 58ced60 - view on LGTM.com

new alerts:

• 2 for Unused format argument

[jnturton]

@cgivre @MFoss19 @estherbuchwalter here we are reading column data types from the format config, where we also specify their start and stop offsets. But this format plugin can also accept data types from a provided schema. So my question is: can we drop the data type information from the format config so that we don't introduce multiple ad-hoc ways of specifying this info? This is genuinely a question because I don't know this subject well, but should we not work with data type specs here exactly the same way we do for CSV (cannot be provided in the format config I don't think)?

[cgivre]

My original understanding of this was that for the fixed width plugin was that it would work in a similar manner to the log regex reader where the user provides the schema in the config, either in the format config or at query time using the table() function.

[paul-rogers]

What you want to do is to resolve the schema at open time, not when parsing. At open time, you can:

• Get the schema from the plugin, if provided, and use that as the schema.
• Else, sample the first line to infer the schema.

Since this is a fixed format, we don't want to rediscover the schema on every line: that costs too much. (Think of the case of reading 100M or 1B rows: optimizing the inner loop is critical.)

[lgtm-com]

This pull request introduces 2 alerts when merging 9d66f91 into 52838ef - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 9b95c45 into 52838ef - view on LGTM.com

new alerts:

• 2 for Unused format argument

[jnturton]

@MFoss19 @estherbuchwalter following some recent chat with @paul-rogers and my last comment here, how about a reduced format config such as the following? The goal is to get to something terse and consistent with what we do for other text formats.

"fixedwidth": {
  "type": "fixedwidth",
  "extensions": [
    "fwf"
  ],
  "extractHeader": true,
  "trimStrings": true,
  "columnOffsets": [1, 11, 21, 31],
  "columnWidths": [10, 10, 10, 10]
}

Column names and types can already come from a provided schema or aliasing after calls to CAST(). Incidentally, the settings above can be overriden per query using a provided schema too.

There's also a part of that wonders whether we could have justified adding our fixed width functionality to the existing delimited text format reader.

[cgivre]

@MFoss19 @estherbuchwalter following some recent chat with @paul-rogers and my last comment here, how about a reduced format config such as the following? The goal is to get to something terse and consistent with what we do for other text formats.

"fixedwidth": {
  "type": "fixedwidth",
  "extensions": [
    "fwf"
  ],
  "extractHeader": true,
  "trimStrings": true,
  "columnOffsets": [1, 11, 21, 31],
  "columnWidths": [10, 10, 10, 10]
}

Column names and types can already come from a provided schema or aliasing after calls to CAST(). Incidentally, the settings above can be overriden per query using a provided schema too.

There's also a part of that wonders whether we could have justified adding our fixed width functionality to the existing delimited text format reader.

@dzamo In this case, I'd respectfully disagree here. In effect, the configuration is providing a schema to the user, similar to the way the logRegex reader works. In this case, the user will get the best data possible if we can include datatypes and field names in the schema, so that they can just do a SELECT * and not have to worry about casting etc.

Let's consider a real world use case: some fixed width log generated by a database. Since the fields may be mashed together, there isn't a delimiter that you can use to divide the fields. You could use however the logRegex reader to do this. That point aside for the moment, the way I imagined someone using this was that different configs could be set up and linked to workspaces such that if a file was in the mysql_logs folder, it would use the mysql log config, and if it was in the postgres it would use another.

My opinion here is that the goal should be to get the cleanest data to the user as possible without the user having to rely on CASTs and other complicating factors.

[jnturton]

Let's consider a real world use case: some fixed width log generated by a database. Since the fields may be mashed together, there isn't a delimiter that you can use to divide the fields. You could use however the logRegex reader to do this. That point aside for the moment, the way I imagined someone using this was that different configs could be set up and linked to workspaces such that if a file was in the mysql_logs folder, it would use the mysql log config, and if it was in the postgres it would use another.

@cgivre This use case would still work after two CREATE SCHEMA statements to set the names and data types, wouldn't it? The schemas would be applied every subsequent query.

My opinion here is that the goal should be to get the cleanest data to the user as possible without the user having to rely on CASTs and other complicating factors.

Let's drop the CASTs, those aren't fun. So we're left with different ways a user can specify column names and types.

1. With a CREATE SCHEMA against a directory.
2. With an inline schema to a table function.
3. With some plugin-specific format config that works for this plugin but generally not for others.

Any one requires some effort, any one gets you to select * returning nice results (disclaimer: is this claim I'm making actually true?) which is super valuable. So shouldn't we avoid the quirky 3 and commit to 1 and 2 consistently wherever we can?

[lgtm-com]

This pull request introduces 2 alerts when merging f9e96fe into 42e7b77 - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 428a512 into 14d96d1 - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 428a2dd into 17f3654 - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 881d465 into 38d0c1d - view on LGTM.com

new alerts:

• 2 for Unused format argument

[lgtm-com]

This pull request introduces 2 alerts when merging 56d8f6e into 38d0c1d - view on LGTM.com

new alerts:

• 2 for Unused format argument

[cgivre]

@MFoss19 @estherbuchwalter I added some review comments. Please make sure the unit tests pass and also that there are no code style violations before pushing to github.

[cgivre]

You can remove this second line. Also, please add e.getMessage() to the message line.

[paul-rogers]

I like that second line (if you mean the message call). Another solution is to change message() to addContext(). This way, we preserve the message from the actual error, and add context to explain the source of the error. Then, as Charles suggested, we don't need the addContext(e.getMessage()) bit.

[cgivre]

This line should be in the open() method.

[cgivre]

For the error message, you don't need to have multiple addContext() calls. The main thing is to pass the errorContext. I would add the e.getMessage() to the message() call.

[cgivre]

Here and elsewhere.

[paul-rogers]

See explanation above. Ideally:

      throw UserException
        .dataReadError(e)
        .addContext("Failed to read input file: {}", split.getPath().toString())
        .addContext(errorContext)
        .addContext("Line Number", lineNum)
        .build(logger);

Thanks for adding the line number: nice touch.

[cgivre]

Why not include this in the loop?

[cgivre]

The next() method needs some work. Really this should be called nextBatch() as the next method returns true when there is more data, to read, false if not.

  @Override
  public boolean next() {
    while (!rowWriter.isFull()) {
      if (!processNextLine()) {
        return false;
      }
    }
    return true;
  }

This method will iterate through the batch of data, and when the rowWriter is full, (IE the batch is full) it will stop reading, BUT the method will return true because there is more data to read. The limit is pushed down in the processNextLine() method.

[cgivre]

Here's where you can check to see whether the user provided a schema or not. You could do something like this:

   if (negotiator.hasProvidedSchema()) {
      TupleMetadata providedSchema = negotiator.providedSchema();
      // Build column writer array
      negotiator.tableSchema(finalSchema, true);
    } else {
      negotiator.tableSchema(buildSchema(), true);
    }

[paul-rogers]

This is done internally in V2. V2 does it that way because it became clear that there is no reason for every reader to have to know to do this same pattern.

[cgivre]

This line should be out of the if statement.

[paul-rogers]

The reason it an be out of the if statement is that the method itself handles null values. Otherwise, the code would be fine as it is if it called close() directly.

[cgivre]

My original understanding of this was that for the fixed width plugin was that it would work in a similar manner to the log regex reader where the user provides the schema in the config, either in the format config or at query time using the table() function.

[cgivre]

Change to true.

[paul-rogers]

But only if the additional work described above is done.

[paul-rogers]

@cgivre, it's been a while since I looked at that stuff. As I recall, the CSV reader was converted, but it is a bit obscure. The unit test also show what can be done, IIRC.

Basically, V2 gets schema from multiple sources:

• Schema info in the plan object (if available in the future)
• Provided schema (the current interim solution to system-provided schema)
• Schema implied by the selected columns (for example, a[1] means a has to be an array.)
• Schema defined by the data source itself (as in Parquet, or in CSV where you can choose the columns array)
• Schema discovered by the traditional "schema on read"

The V2 SchemaNegotiator provides the needed methods. V2 will then come up with the final schema. V2 lets you read the entire row (columns a, b, c, say) even if the query wants only column a: V2 silently discards the unwanted columns.

[paul-rogers]

This is done internally in V2. V2 does it that way because it became clear that there is no reason for every reader to have to know to do this same pattern.

[paul-rogers]

I like that second line (if you mean the message call). Another solution is to change message() to addContext(). This way, we preserve the message from the actual error, and add context to explain the source of the error. Then, as Charles suggested, we don't need the addContext(e.getMessage()) bit.

[paul-rogers]

You can lose a line here: read the next line, but the writer is full, so we discard it. As @cgivre suggests:

while (!writer.isFull()) {
  String line = reader.readLine();
  if (line == null) {
    break;
  }
  writer.start();
  parseLine(line, writer);
  writer.save();
}

[paul-rogers]

See explanation above. Ideally:

      throw UserException
        .dataReadError(e)
        .addContext("Failed to read input file: {}", split.getPath().toString())
        .addContext(errorContext)
        .addContext("Line Number", lineNum)
        .build(logger);

Thanks for adding the line number: nice touch.

[paul-rogers]

But only if the additional work described above is done.

[paul-rogers]

Generally, for something like this, you can use a LIMIT 2 and only check the first row or two. The code is not going to change column order after the 10th row!

[paul-rogers]

Is the blank line intentional? Kind of hard to document data files. You could allow comments (lines that start with #, say, but there is always someone who has valid data that starts with #.) Another idea would be to add a "explanation.txt" file that explains the purpose of each of the data files.

[paul-rogers]

Also, I'd hoped to see all manner of invalid files:

• Truncated line (not just an entirely blank line)
• String where a number should be.
• Badly formatted date or time.

Such files will ensure that the error handling works and will raise that perpetual question: if we're a hundred million lines into a fixed-width file, and we hit an error, should we ignore that line and move to the next, or should we fail the query?

[MFoss19]

It is intentional to test what would happen if there was a blank line in a user input file. Good idea to add the explanation.txt file and additional invalid files.

[lgtm-com]

This pull request introduces 1 alert when merging bf6a16c into 4e97f5c - view on LGTM.com

new alerts:

• 1 for Unused format argument