Copilot commented on code in PR #2801:
URL: https://github.com/apache/sedona/pull/2801#discussion_r3005808988
##########
snowflake/src/main/java/org/apache/sedona/snowflake/snowsql/ddl/DDLGenerator.java:
##########
@@ -90,34 +94,33 @@ public static void main(String[] args) {
argMap.put("stageName", "");
appRoleName = argMap.getOrDefault("appRoleName", "app_public");
if (!argMap.containsKey("appRoleName")) {
- System.out.println(
+ log.info(
"-- AppRoleName is required when isNativeApp is true. If not
provided, default to app_public");
}
stageName = "";
- System.out.println("-- Generating DDL for Snowflake Native App");
- System.out.println("CREATE APPLICATION ROLE IF NOT EXISTS " +
appRoleName + ";");
- System.out.println("CREATE OR ALTER VERSIONED SCHEMA sedona;");
- System.out.println("GRANT USAGE ON SCHEMA sedona TO APPLICATION ROLE " +
appRoleName + ";");
+ log.info("-- Generating DDL for Snowflake Native App");
+ log.info("CREATE APPLICATION ROLE IF NOT EXISTS {};", appRoleName);
+ log.info("CREATE OR ALTER VERSIONED SCHEMA sedona;");
+ log.info("GRANT USAGE ON SCHEMA sedona TO APPLICATION ROLE {};",
appRoleName);
} else {
// If isNativeApp is false, set stageName to the provided value. Also
set appRoleName to empty
// string since it is not needed.
// If stageName is not provided, default to @ApacheSedona. The name must
start with @.
isNativeApp = false;
appRoleName = "";
- System.out.println(
- "-- IsNativeApp is false. Generating DDL for User-Managed Snowflake
Account");
+ log.info("-- IsNativeApp is false. Generating DDL for User-Managed
Snowflake Account");
stageName = argMap.getOrDefault("stageName", "@ApacheSedona");
if (!stageName.startsWith("@")) {
- System.out.println("-- StageName must start with @");
+ log.error("-- StageName must start with @");
exit(0);
}
}
try {
- System.out.println("-- UDFs --");
- System.out.println(
+ log.info("-- UDFs --");
+ log.info(
String.join("\n", UDFDDLGenerator.buildAll(argMap, stageName,
isNativeApp, appRoleName)));
- System.out.println("-- UDTFs --");
- System.out.println(
+ log.info("-- UDTFs --");
+ log.info(
String.join(
"\n", UDTFDDLGenerator.buildAll(argMap, stageName, isNativeApp,
appRoleName)));
Review Comment:
The generated DDL is now emitted via SLF4J `log.info(...)`. This tool is
documented to be used with stdout redirection to create a pure SQL script
(e.g., `java -jar ... > sedona-snowflake.sql`), but logger output may be
suppressed (no config / INFO disabled) or prefixed with timestamps/levels,
corrupting the SQL file. Consider writing the DDL statements to stdout directly
and using the logger only for diagnostics.
##########
snowflake/src/main/java/org/apache/sedona/snowflake/snowsql/ddl/DDLGenerator.java:
##########
@@ -53,25 +57,25 @@ public static Map<String, String> parseArgs(String[] args) {
try {
assert argMap.containsKey(Constants.GEOTOOLS_VERSION);
} catch (AssertionError e) {
- System.out.println("Missing required arguments");
+ log.error("Missing required arguments");
printUsage();
}
return argMap;
Review Comment:
`parseArgs` uses a Java `assert` to enforce `--geotools-version`. Assertions
are disabled by default in production JVMs, so this validation will silently do
nothing and the tool will continue with missing required args. Replace this
with an explicit check (and exit with non-zero status) so the CLI behaves
correctly without `-ea`.
##########
snowflake/src/main/java/org/apache/sedona/snowflake/snowsql/ddl/DDLGenerator.java:
##########
@@ -53,25 +57,25 @@ public static Map<String, String> parseArgs(String[] args) {
try {
assert argMap.containsKey(Constants.GEOTOOLS_VERSION);
} catch (AssertionError e) {
- System.out.println("Missing required arguments");
+ log.error("Missing required arguments");
printUsage();
}
return argMap;
}
public static void printUsage() {
- System.out.println("Usage: java -jar
snowflake/target/sedona-snowflake-1.5.1.jar [options]");
- System.out.println("Must have Arguments");
- System.out.println(" --geotools-version <version>");
- System.out.println("Optional have Arguments");
- System.out.println(" --schema <schema> register functions to this
schema. Default to sedona");
- System.out.println(
+ log.info("Usage: java -jar snowflake/target/sedona-snowflake-1.5.1.jar
[options]");
+ log.info("Must have Arguments");
+ log.info(" --geotools-version <version>");
+ log.info("Optional have Arguments");
+ log.info(" --schema <schema> register functions to this schema. Default
to sedona");
+ log.info(
" --stageName <stageName> snowflake stage name to upload jar files.
Not needed if isNativeApp is true");
- System.out.println(
+ log.info(
" --isNativeApp <true/false> whether to generate DDL for Snowflake
Native App. Default to false");
- System.out.println(
+ log.info(
" --appRoleName <appRoleName> application role name. Required when
isNativeApp is true. Default to app_public");
- System.out.println(" --h Print this help message");
+ log.info(" --h Print this help message");
exit(0);
Review Comment:
`printUsage()` advertises `--h`, but the argument parser checks for `-h`.
This mismatch makes the documented help flag not work; please align the usage
text and accepted flags (supporting both `-h` and `--help` would be ideal).
##########
snowflake/src/main/java/org/apache/sedona/snowflake/snowsql/ddl/DDLGenerator.java:
##########
@@ -53,25 +57,25 @@ public static Map<String, String> parseArgs(String[] args) {
try {
assert argMap.containsKey(Constants.GEOTOOLS_VERSION);
} catch (AssertionError e) {
- System.out.println("Missing required arguments");
+ log.error("Missing required arguments");
printUsage();
}
return argMap;
}
public static void printUsage() {
- System.out.println("Usage: java -jar
snowflake/target/sedona-snowflake-1.5.1.jar [options]");
- System.out.println("Must have Arguments");
- System.out.println(" --geotools-version <version>");
- System.out.println("Optional have Arguments");
- System.out.println(" --schema <schema> register functions to this
schema. Default to sedona");
- System.out.println(
+ log.info("Usage: java -jar snowflake/target/sedona-snowflake-1.5.1.jar
[options]");
+ log.info("Must have Arguments");
+ log.info(" --geotools-version <version>");
+ log.info("Optional have Arguments");
+ log.info(" --schema <schema> register functions to this schema. Default
to sedona");
+ log.info(
" --stageName <stageName> snowflake stage name to upload jar files.
Not needed if isNativeApp is true");
- System.out.println(
+ log.info(
" --isNativeApp <true/false> whether to generate DDL for Snowflake
Native App. Default to false");
- System.out.println(
+ log.info(
" --appRoleName <appRoleName> application role name. Required when
isNativeApp is true. Default to app_public");
- System.out.println(" --h Print this help message");
+ log.info(" --h Print this help message");
exit(0);
}
Review Comment:
`printUsage()` now logs at INFO. For CLI help/usage (especially when users
redirect stdout to a file), it’s safer to print usage/errors to stderr (and
exit with a non-zero status for invalid args) so the SQL output stream stays
clean and help text is still visible.
--
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]
