Do we want to fail if the file name is just an extension? I think this is a reasonable choice because the only weird case I can imagine is setting your file name to metadata.json. All the other extensions are pretty much useless without another part to the name.

Yeah, let's follow the common definition of an extension being "the bit after the last dot" (e.g., commons-io).

I think my test cases with parquet as filename were silly. Removed those

actually I would consider changing this part to

public static FileFormat fromFileName(CharSequence filename) {
    for (FileFormat format : VALUES) {
      if (null != filename && filename.toString().endsWith(format.ext)) {
        return format;
      }
    }

    return null;
  }

then we wouldn't need to deal with negative indexes and such and all places that call this method already pass an actual String, so calling toString() is cheap

I think if we did that we should just make a new version of the method, I don't think it's great to change the underlying perf semantics of the method in the API module. If it takes CharSequence we shouldn't change it to a string.

#12300 mentions that there's an issue with file names where the prefix is shorter than the file format suffix, but those things aren't tested here. The params should be updated to include x.puffin / x.orc / x.parquet / x.avro to cover that

@nastra our longest "extension" is metadata.json so a lot of these are too short for that :)

I've checked locally but the test cases that expect a valid file format all pass without the fix (meaning that they don't reproduce the original issue that was reported).
The only test cases that fail is where we're expecting a null file format.

That's why I'm suggesting to also test x.orc and so on, since all of those use cases that expect a valid file format won't fail and reproduce the original issue because they never actually reach the metadata.json file format during iteration, since they are matching a different file format earlier and thus the original issue isn't reproduced

Can you double-check? I've undone the fix locally and get these test failures, all with out-of-bounds indexes:

E.g., for the file.csv case:

• It goes through the list of file formats in enum order, the last is metadata.json with 13 characters
• From length of file.csv (8 characters), subtract 13 characters for .metadata.json and 1 for the dot
• Your start index is -6 and you get an IndexOutOfBoundsException

It does the checks in order, so if you get a hit on "orc" you won't fail at "metadata.json"

Can you double-check? I've undone the fix locally and get these test failures, all with out-of-bounds indexes:

E.g., for the `file.csv` case:

• It goes through the list of file formats in enum order, the last is metadata.json with 13 characters
• From length of file.csv (8 characters), subtract 13 characters for .metadata.json and 1 for the dot
• Your start index is -6 and you get an IndexOutOfBoundsException

Yes I'm aware of that. My point is that we should just add some test cases where we're expecting a valid FileFormat to show that the same issue happens e.g. with x.orc too and not just unsupported file formats like csv.

@rshkv can you please add

        {"x.parquet", FileFormat.PARQUET},
        {"x.puffin", FileFormat.PUFFIN},
        {"x.orc", FileFormat.ORC},
        {"x.avro", FileFormat.AVRO},

in the right order to the argument list?

Ah, I get you now. We'd hit out of bounds if x.avro was compared against .parquet which happens because PARQUET is before the AVRO enum value.

Updated

we should also test where the filename is null or an empty string

can you please add @SuppressWarnings("unused") right above this line