Nit: Can you please reformat with 2 space indentation.

Done

Nit: Please remove extra whitespace, here and elsewhere.

Done

Please remove any calls to System.out.println in unit tests.

Done

These last two lines are not necessary.

Done

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?

There is a small issue with the next() function as written. You define maxRecords but don't do anything with it. The maxRecords is the LIMIT which gets pushed down from the query. The idea being that if a user does a SELECT ... LIMIT 10 your reader should stop reading as soon as that limit is reached. Which means that the next function should return false when the limit has been reached. The good news is that your writer object actually has a method called limitReached(<maxRecords>) which will track this for you.

Another thing you might consider doing which would clean up the code a bit would be to make the parseLine method return true if there is more data to read, false if not. Also move the writer start and end to that method, then you could have a next method that looks like this:

@Override
  public boolean next() {
    recordCount = 0;
    while (!writer.isFull()) {
      if (!parseLine(writer)) {
        return false;
      }
    }
    return true;
  }

Fixed use of MaxRecords in next() function.

Still looking at moving writer start and end to next.

I'd suggest tightening this a bit. Instead of capturing a generic Exception, perhaps use specific exceptions with different error messages. Alternatively just put the try/catch around the line(s) in which data is being read.

Modified to capture IOException. At first, had done this deliberately since parseLine would throw Runtime Exception, but captured this to rethrow as IOException for clarity.

Consider using Autoclosables here. That will do all the error handling for you.

 @Override
  public void close() {
    if (fsStream != null) {
      AutoCloseables.closeSilently(fsStream);
      fsStream = null;
    }
  }

Done

See comment above but I'd recommend making this method return a boolean value.

Nit: Please remove excess whitespace here and elsewhere.

Done

Please convert this to a UserException.

It looks like we are only supporting INT, VARCHAR, DATE, TIME and TIMESTAMP. Do we want to support a few other data types such as LONG or DOUBLE?

Yes, will add LONG and DOUBLE. Also looking to add FLOAT and DECIMAL.

Please add Apache license to all files.

Done

Please remove struts here and elsewhere.

For all serializable classes in Drill, I would recommend overriding equals, hashcode, and toString() methods.

Done

I think this actually might be blocksplittable.

Drill has no config builder UI, users have to create JSON configs by hand. We've found it is helpful to keep field names short (Go-style.) So, perhaps:

• dataType → type
• fieldName → name
• dateTimeFormat long, but OK
• startIndex → index
• fieldWidth → width

Note that, since this is a FieldConfig we don't need the field prefix.

Also, it can help readers if the "primary key" fields are first, so perhaps change the order to

• name
• index
• width
• type
• dateTimeFormat

Modified names and order.

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.

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.

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.

Nit: GitHub is complaining about a lack of final newlines.

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.)

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.

@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?

@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.

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.

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.

@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)?

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.

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.)

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

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.

This line should be in the open() method.

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.

Here and elsewhere.

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.

Why not include this in the loop?

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.

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);
    }

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.

This line should be out of the if statement.

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.

Change to true.

But only if the additional work described above is done.

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();
}

Fixed-width files seem perfect for HDFS splits.

So there is a trick here for old-school HDFS systems (if anyone still runs them.) A large file will be split across HDFS nodes, often at 256MB boundaries. A reader that supports "splits" has to handle the fact that node 1 will read the first 256 MB, node 2 the next 256 MB, and so on. Standard HDFS stuff.

This means the reader has to accept the offset and scan for the first record separator after that start point. If the fixed-width records are newline-terminated, that means finding the next newline.

Also, the reader has to stop after it finds the record terminator after its assigned split. (On HDFS, that means part of the last record will be read from a remote HDFS node.)

Further, it means that line numbers, as counted here, are relative: they are from the start of the current split (since node 2, reading the second split, doesn't know the number of records in the first split.)

The CSV reader handles this busy-work, but it a way that is a bit hard to follow as an example, sadly.

Just a high-level design note: we now have multiple plugin configs that ask the user to use ad-hoc formats for specifying a per-file schema. This is not how storage plugins were meant to be used, but it is all we have.

Over time, we need a better solution. The provided-schema is a start: it provides a single, well-defines syntax for schema. But, it is a bit limited in handling per-format specific, such as width. (There are extension properties for this kind of information, but that's a bit fiddly.)

We need a way to provide the schema of a file simply as a file the user creates that sits along side the data file. Something like a schema.json file if all files in a directory have the same schema, or a foo.json if foo.txt has a distinct schema.

Support for such a file should be added to the V2 mechanism. (Just use the new format in place of the provided schema.)

This makes the user do the math. Given indexes, we can compute the width. Given widths, we can compute the index. The one missing piece might the the width of the field separator (if any).

No need for this tag: Jackson doesn't know what to do with this method anyway.

This is just a bit dangerous. We validate on deserialize. This seems like a great idea: we do the deserialize in the UI when the user saves the config. But, we also do it on system start. If the deserialize fails there, Drill won't start and it takes a long time to figure out why.

We don't have a good answer for config validation. I'd suggest adding a validate() method that we call:

• When saving a config from the UI
• When planning a query using a config

But, allow an invalid config to be stored. Otherwise, unless everything is perfect, nothing can be saved. And, JSON, in a text editor, is a horrible way to write complex config such as this one.

This message will (I hope) be shown to the poor user trying to get a config right in the Drill web UI. Can we be more specific? Such as, "Name is not valid. Only letters allowed."

But, then, why don't we allow numbers or underscores? "Field_3"? If we allowed that, you could use the Java method. Or, extend the pattern.

Also, it is cleaner to use name = name.strip() to remove both leading and trailing whitespace so we don't leave whitespace in the name. Otherwise, all users have to know to do their own strip() call.

The regex here says that the first character must be a letter, and the rest (\w*) must be alphabetical, numerical, or the underscore.

Nit: missing newline.

Fixed

Nit: spacing after comma. Remove unwanted format as per previous line.

Fixed spacing and removed unnecessary dateTimeFormats

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!

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.

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?

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.