vlsi commented on pull request #2621: URL: https://github.com/apache/calcite/pull/2621#issuecomment-1000265445
@woonsan , see https://github.com/apache/calcite/commit/2f33a0c57b7b7e77b8193d0fff1e3531119aee0a#diff-7350bd9b77a86a4eef5e2b2b000cd9c9ae8bf30f1264222c1fed43a688b967e4 You add the following documentation note: > resource URI like `classpath:/com/example/calcite/model.yaml` However, the URI standard does have a way to encode special symbols. What is missing is that you mention URI, and you miss to clarify how special symbols (e.g. `+`, `:`, `/`, `space`, emojis, whatever) should be represented in that URI. Shoud it be `classpath:/com/hello%20world.yml` or `classpath:/com/hello world.yml`? ---- Side note: when you add `.getResource...`, you need a class loader. Sometimes Calcite's classloader is fine, and sometimes `threadcontext classloader` would be slightly better. So I would suggest you allow both options (e.g. via different prefixes or something). --- All in all, I do not block the PR in any way (even though I think it should be improved before the merge), and I feel I won't have time to review it later. -- 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]
