[Python] Fix issue with dtype parameter in the read_csv method.

[Tishj]

This PR fixes #8209, #8421, #8434

Relations are bound twice, one to get the names+types, and the second time to execute.
the dtype parameter was only handled after the first bind, so its effects were not visible in the columns and types attributes of the produced relation.

Also now we only auto_detect during the first phase, capturing the options detected by the sniffer and forwarding them to the second bind as named parameters.

i.e if a quote was detected, we forward this option as read_csv(quote=<detected_quote>)

[Mytherin]

Thanks for the PR!

I had a quick look at the problem and I think it goes a bit deeper than what this fix addresses. I think the problem should actually be solved in ReadCSVRelation. The issue is here in the constructor:

ReadCSVRelation::ReadCSVRelation(const std::shared_ptr<ClientContext> &context, const string &csv_file,
                                 BufferedCSVReaderOptions options, string alias_p)
    : TableFunctionRelation(context, "read_csv_auto", {Value(csv_file)}, nullptr, false), alias(std::move(alias_p)),
      auto_detect(true) {

	if (alias.empty()) {
		alias = StringUtil::Split(csv_file, ".")[0];
	}

	// Force auto_detect for this constructor
	options.auto_detect = true;
	BufferedCSVReader reader(*context, std::move(options));

	auto &types = reader.GetTypes();
	auto &names = reader.GetNames();
	for (idx_t i = 0; i < types.size(); i++) {
		columns.emplace_back(names[i], types[i]);
	}

	AddNamedParameter("auto_detect", Value::BOOLEAN(true));
}

The options that are set get passed in to the BufferedCSVReader, which then uses those options to run auto-detection and obtain the columns and types. This means only flags set in the BufferedCSVReaderOptions are considered for the initial binding. However, any parameters provided here are subsequently lost.

Afterwards, the actual table function is constructed and AddNamedParameter is called to add new parameters as named parameters. These parameters are then considered for the actual execution of read_csv.

All of this is to say this is quite messy - we have two avenues of providing options (BufferedCSVReaderOptions and AddNamedParameter) and both affect the relation in different ways. In addition, auto-detection is run multiple times unnecessarily.

Ideally we would only have one way of providing options (AddNamedParameter - which could be used with the BufferedCSVReaderOptions using the SetReadOption function). The auto-detection would run once when creating the relation, and the found columns and types would be saved in the relation (by for example adding a dtypes=[...] parameter). This would prevent the auto-detection from running again in subsequent iterations.

[Tishj]

Yea I agree this is really messy, and error-prone.

It's not immediately clear how to fix this in the ReadCSVRelation, but I'll have a look soon using the helpful pointers you provided 👍

[Mytherin]

My suggested fix would be:

• Make ReadCSVRelation take as parameter an unordered_set<string, Value> instead of a BufferedCSVReaderOptions
• Use AddReadOption to populate the BufferedCSVReaderOptions for the auto-detection pass
• In the ReadCSVRelation itself - call AddNamedParameter with the provided parameters (in unordered_set<string, Value>), plus whatever was found during the auto-detection process (i.e. also populate dtypes, header, delimiter, with what was found during auto-detection) and set auto-detection to false

Then in the Python method only populate the unordered_set<string, Value>, instead of populating both BufferedCSVReaderOptions and using AddNamedParameter

[Mytherin]

I think #8421 is related to this as well

[Mytherin]

Thanks for the changes! LGTM now

[Tishj]

@Mytherin I think this can also be merged

CI both here and on my fork passed
I also just merged with main and there are no conflicts

[Mytherin]

Thanks!