andygrove commented on PR #64: URL: https://github.com/apache/datafusion-spark/pull/64#issuecomment-6823857489
Thanks for the PR, the logic looks right to me. One small observation on the ANSI-mode error for the port case. `http://spark.apache.org:99999/path` currently errors with `Execution error: InvalidPort`. That comes from `exec_datafusion_err!("{e:?}")` in `ParseUrl::parse`, so it is the Debug form of `url::ParseError`. The other invalid-URL case gives the clearer `The url is invalid: ... SQLSTATE: 22P02` message. The new slt test pins the `InvalidPort` text. It would be nicer to produce the same `The url is invalid` message for every parse failure, and then the test wouldn't depend on a Debug string. This is existing behavior, so it doesn't need to block this PR. A follow-up would be fine, or you could fold it in here if you prefer. -- 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]
