fix: validate that either a ndjson or tsv file was given as input

[Taepper]

resolves #564

Summary

We did not check that either a ndjson or tsv file is actually specified as the input.

The resulting duckdb error is not parsable for users. Instead we now return a proper error message, also hinting at the possible CLI arguments that can be given instead of amending the preprocessing_config

PR Checklist

• All necessary documentation has been adapted or there is an issue to do so.
• The implemented feature is covered by an appropriate test.

[github-actions]

This is a preview of the changelog of the next release:

0.2.19 (2024-09-12)

Bug Fixes

• validate that either a ndjson or tsv file was given as input (6870236)

[corneliusroemer]

How is it possible that duckdb threw such a cryptic/mojibake error?

This looks a bit like out of bounds access, which isn't great.

Validating is good, but I wonder how the error could end up so mangled. Can you point me to the place where duckdb crashed/threw?

[corneliusroemer]

Here you could list the various ways in which they can be passed, but not super important (CLI, env, config file)

[Taepper]

I removed it after the above comment.

We have them described in the README. Although the part about default values and the configs therefore being optional is unfortunately no longer valid as of my refactoring here...

[corneliusroemer]

In the change log entry I suggest to:

• link to the GitHub issue that reported the bug
• link to the PR that fixed it (this one here)
• mention that this was about preprocessing, I don't think that's clear from the given summary

[corneliusroemer]

If there is a bug in the CLI option part, might make sense to add some basic tests here (E2E, once you get to it :) )

this stuff should be super easy to test for regressions

[Taepper]

Can you point me to the place where duckdb crashed/threw?

The actual part that was broken / where the gibberish was generated is an unchecked optional access here. Note that the -> symbol implicitly calls .value() on the optional.

I will add comments / additional action points to #562

[corneliusroemer]

I see, and how come it produced mojibake? Because nullpointer access and C++ being unsafe language? I've only done stuff like that in Rust and there it would throw instead of producing gibberish.

[Taepper]

I also thought it would throw an exception (which would still puzzle the end-user) as is documented for the value function here.

But it turns out the shorthand syntax is just undefined behaviour :(

[fengelniederhammer]

Probably this was the evil line?

LAPIS-SILO/src/silo/preprocessing/preprocessor.cpp

Line 116 in c34fa1f

buildMetadataTableFromFile(*preprocessing_config.getMetadataInputFilename());