davidm-db commented on PR #49427: URL: https://github.com/apache/spark/pull/49427#issuecomment-2630514514
Minor comments, otherwise LGTM. I have one concern regarding all the case insensitive checks in the code - for example, in `SqlScriptingLabelContext.checkLabels` we are doing conversion to lowercase in the place of a check, rather than relying on propagating uppercase/lowercase strings and then having them in the correct case at the time of a check (which we are doing here). The approach we have with labels seems more robust and localized. Here (in `visitCompoundBodyImpl`, for example, instead of propagating uppercase conditions, we would propagate just the input values, and then when adding to `conditions` map we can do a lowercase conversion and then each time we check if the condition is in the map, we do the conversion as well. This way, it's localized to the place where we do the checks and if something changes in the future we don't need to rely that someone will take care of this, it would simply work. Thoughts? -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
