gianm commented on PR #13360: URL: https://github.com/apache/druid/pull/13360#issuecomment-1343864369
> @gian, thanks for the comments! A few answers: > > 1. Yes, it is possible to use the syntax with `extern`. I've not wired that up, but doing so should be straightforward. If you do choose to wire it up, that'd be nice — about 33% nicer for `EXTERN` IMO 🙂 > 2. It is on the to-do list to use an array type for list arguments. Might be in the next PR. Sounds good! Since it'll affect the user-facing syntax, if this is part of the plan, it'll be best to get that in before the next release. Otherwise we'll have compatibility to deal with. > 3. So, on the docs, my intent was to refer to the existing docs rather than repeating stuff. I'll change it to use whatever names we use for those things elsewhere. Sounds good. > 4. The argument names are deliberately shorter than the JSON field names. Folks will be typing this, the name space is small, so no need for the verbose names we use in JSON. The JSON names were not selected with typing usability in mind. `httpAuthenticationUsername` Really? Ah. Nevertheless I feel we are better served by consistency here than by brevity. People get tripped up when the same thing has two different names in two different contexts. An option: we could have the JSON one accept the shorter name too: the JSON object could accept both the long and short name, and write out the old name on serialization for compatibility reasons. Then, we could document the short name for both. Thoughts, preferences? > 5. Thanks for the note on JSON. Is the `keepNullColumns` meant to be supported? If so, we can clean up the JSON input format docs, and I'll reference that here. If it is _not_ supported, I'll remove the function arg. (There are some new JSON format args that I've not yet added for nested types.) I'm not sure. I don't know why it's not documented. IMO, unless there's good reason to document it now, it's better to leave it out of the table-format function, because often things are undocumented because they aren't meant to be supported parameters for end users. > 6. I guess I disagree with the analysis that the functionality is not broken. If I set my `baseDir` to "/tmp", and my files to "x.csv", I expect Druid to read "/tmp/x.csv" not "/Users/paul/druid/x.csv". Not all local input files are relative to Druid's product directly. That said, thanks for fixing the issue. I'll rerun the tests with your fix and correct the wording. Ah. I see what you mean. I don't think it's actually supposed to work that way, is the thing! Looking at the implementation of LocalInputSource, it seems like the intent is `baseDir` + `filter` are one thing, and `files` are another thing, and they aren't connected. That is, the intent is that relative-path `files` are interpreted relative to the Druid working directory, not the provided `baseDir`. Would definitely be good to clarify this in the documentation, since I don't think it's clear. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
