Add the ability to access file_path in FileOpener

[stephaniewang526]

This allows us to support multiple secrets and do path matching, etc.

[Mytherin]

Thanks for the PR!

This will not work correctly as the FileOpener is a shared structure (that lives in the ClientData). If multiple files are opened in different threads because of e.g. UNION pipelines that query different files this will result in a race condition. This should be solved in a different manner, e.g. by adding the file path as a parameter to TryGetCurrentSetting or so.

[stephaniewang526]

Thanks for the review. I'm not so sure where/how we could get and then set the file_path (in this case, glob_pattern) in TryGetCurrentSetting and how it helps with the concurrency problem? I tried a different way (where a new ScopedOpener object is constructed and lives for the scope/duration of just the Glob method). Please let me know if further changes are needed. Thanks!

[Mytherin]

I'm not a big fan of this solution as I can see this becoming tricky w.r.t. memory management of the multiple openers as you are returning a raw pointer and not an owning pointer - I think a cleaner solution would be to expand TryGetCurrentSetting in the FileOpener with a new parameter (FileOpenerInfo or so) that contains a file path and potentially other info (e.g. whether this is a Glob or an OpenFile or so).

[rjatwal]

Can make new argument an optional_ptr and default it to a nullptr for most subclasses.

[rjatwal]

Looking at usage again. It might be cleaner to have two virtual TryGetCurrentSetting methods one with the FileOpenerInfo and one without. This way users can only provide FileOpenerInfo when it makes sense, and avoids potentially confusing nullptrs

[stephaniewang526]

Okay makes sense!

[rjatwal]

Can default this method to disregard FileOpenerInfo to make life easier on library writer (though I am personally inclined towards forcing libraries to be explicit).

[rjatwal]

Should this default to calling the none FileOpenerInfo method i.e.

bool FileOpener::TryGetCurrentSetting(const string &key, Value &result, FileOpenerInfo &info) {
	return this->TryGetCurrentSetting(key, value);
}

[stephaniewang526]

Yes. Corrected.

[stephaniewang526]

Thanks for the pointer. I made another attempt. Please let me know if this makes sense.

[Mytherin]

Thanks for the fixes! LGTM. Could you just have a look at the failing CI?

[Mytherin]

Thanks! LGTM