[GitHub] [calcite] vlsi commented on pull request #2621: [CALCITE-4908] Support classpath resource URI as model URI

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org