kasakrisz commented on code in PR #5182:
URL: https://github.com/apache/hive/pull/5182#discussion_r1557648667
##########
hplsql/src/main/java/org/apache/hive/hplsql/Var.java:
##########
@@ -601,7 +602,7 @@ else if (type == Type.STRING) {
return (String)value;
}
else if (type == Type.DATE) {
- return String.format("DATE '%s'", value);
+ return ((Date)value).toString();
Review Comment:
Is it safe to remove the `DATE` keyword? Let's say we have `2024-04-09`. Is
it a string or a date literal?
##########
hplsql/src/main/java/org/apache/hive/hplsql/Expression.java:
##########
@@ -565,7 +575,11 @@ public void
operatorConcatSql(HplsqlParser.Expr_concatContext ctx) {
sql.append("CONCAT(");
int cnt = ctx.expr_concat_item().size();
for (int i = 0; i < cnt; i++) {
- sql.append(evalPop(ctx.expr_concat_item(i)).toString());
+ String concatStr = evalPop(ctx.expr_concat_item(i)).toString();
+ if (!concatStr.startsWith("'")) {
Review Comment:
AFAIK `concat` expects string parameters. The actual parameters can be a
* string literals which needs quotation
* or an string typed expressions which should not be quoted
Could you please share an example when quotation is missing but required at
this point?
##########
hplsql/src/main/java/org/apache/hive/hplsql/Stmt.java:
##########
@@ -787,8 +787,19 @@ public Integer
insertValues(HplsqlParser.Insert_stmtContext ctx) {
for (int i = 0; i < rows; i++) {
HplsqlParser.Insert_stmt_rowContext row
=ctx.insert_stmt_rows().insert_stmt_row(i);
int cols = row.expr().size();
- for (int j = 0; j < cols; j++) {
- String value = evalPop(row.expr(j)).toSqlString();
+ for (int j = 0; j < cols; j++) {
+ Var var = evalPop(row.expr(j));
+ String value = null;
+ if (var.type == Type.TIMESTAMP) {
+ value = String.format("TIMESTAMP '%s'", var.toString());
+ } else if (var.type == Type.DATE) {
+ value = String.format("DATE '%s'", var.toString());
+ } else {
+ value = var.toSqlString();
+ }
+ if (value.startsWith("''")) {
Review Comment:
Which case can we have "double" quoted value?
##########
hplsql/src/main/java/org/apache/hive/hplsql/functions/FunctionDatetime.java:
##########
@@ -162,6 +174,35 @@ void toTimestamp(HplsqlParser.Expr_func_paramsContext ctx)
{
}
}
+ /**
+ * FROM_UNIXTIME() function (convert seconds since 1970-01-01 00:00:00 to
timestamp)
+ */
+ void fromUnixtime(HplsqlParser.Expr_func_paramsContext ctx) {
+ int cnt = BuiltinFunctions.getParamCount(ctx);
+ if (cnt == 0) {
+ evalNull();
+ return;
+ }
+ Var value = evalPop(ctx.func_param(0).expr());
+ if (value.type != Var.Type.BIGINT) {
+ Var newVar = new Var(Var.Type.BIGINT);
+ value = newVar.cast(value);
+ }
+ long epoch = value.longValue();
+ String format = "yyyy-MM-dd HH:mm:ss";
+ if (cnt > 1) {
+ format =
Utils.unquoteString(evalPop(ctx.func_param(1).expr()).toString());
+ }
+ evalString(Utils.quoteString(new SimpleDateFormat(format).format(new
Date(epoch * 1000))));
Review Comment:
Can the newer Java Date Time API be used? `java.time.*`
##########
hplsql/src/main/antlr4/org/apache/hive/hplsql/Hplsql.g4:
##########
@@ -1563,7 +1563,7 @@ non_reserved_words : // Tokens that
are not reserved words
| T_TO
| T_TOP
| T_TRANSACTION
- | T_TRIM
+ // | T_TRIM
Review Comment:
Is `trim` a reserved word?
##########
hplsql/src/main/java/org/apache/hive/hplsql/Expression.java:
##########
@@ -110,7 +110,17 @@ else if (ctx.interval_item() != null) {
}
else {
visitChildren(ctx);
- sql.append(exec.stackPop().toString());
+ Var value = exec.stackPop();
+ if (value.type == Type.NULL && sql.toString().length() == 0) {
+ exec.stackPush(new Var());
+ return;
+ } else if (exec.buildSql && value.type == Type.DATE) {
+ sql.append(String.format("DATE '%s'", value.toString()));
+ } else if (exec.buildSql && value.type == Type.TIMESTAMP) {
+ sql.append(String.format("TIMESTAMP '%s'", value.toString()));
+ } else {
+ sql.append(value.toString());
+ }
Review Comment:
Can `Var.toString()` be used instead? it seems that not just date and
timestamp needs custom string format:
https://github.com/apache/hive/blob/be857bf83c00e41a315a7cc8e013832cf169ef18/hplsql/src/main/java/org/apache/hive/hplsql/Var.java#L590-L618
##########
hplsql/src/main/java/org/apache/hive/hplsql/Stmt.java:
##########
@@ -787,8 +787,19 @@ public Integer
insertValues(HplsqlParser.Insert_stmtContext ctx) {
for (int i = 0; i < rows; i++) {
HplsqlParser.Insert_stmt_rowContext row
=ctx.insert_stmt_rows().insert_stmt_row(i);
int cols = row.expr().size();
- for (int j = 0; j < cols; j++) {
- String value = evalPop(row.expr(j)).toSqlString();
+ for (int j = 0; j < cols; j++) {
+ Var var = evalPop(row.expr(j));
+ String value = null;
+ if (var.type == Type.TIMESTAMP) {
+ value = String.format("TIMESTAMP '%s'", var.toString());
+ } else if (var.type == Type.DATE) {
+ value = String.format("DATE '%s'", var.toString());
+ } else {
+ value = var.toSqlString();
+ }
Review Comment:
How about `var.toString()`?
##########
hplsql/src/main/java/org/apache/hive/hplsql/functions/FunctionMisc.java:
##########
@@ -59,6 +64,43 @@ public void register(BuiltinFunctions f) {
void activityCount(HplsqlParser.Expr_spec_funcContext ctx) {
evalInt(Long.valueOf(exec.getRowCount()));
}
+
+ /**
+ * CAST function
+ */
+ void cast(HplsqlParser.Expr_spec_funcContext ctx) {
+ if (ctx.expr().size() != 1) {
+ evalNull();
+ return;
+ }
Review Comment:
Does it mean that the user specified wrong parameters? Should we throw
exception?
##########
hplsql/src/main/java/org/apache/hive/hplsql/functions/FunctionMisc.java:
##########
@@ -211,6 +269,16 @@ public void partCount(HplsqlParser.Expr_spec_funcContext
ctx) {
query.close();
}
+ public void modulo(HplsqlParser.Expr_func_paramsContext ctx) {
+ if (ctx.func_param().size() == 2) {
+ int a = evalPop(ctx.func_param(0).expr()).intValue();
+ int b = evalPop(ctx.func_param(1).expr()).intValue();
+ evalInt(a % b);
+ } else {
+ evalNull();
Review Comment:
Should exception be thrown?
##########
hplsql/src/main/java/org/apache/hive/hplsql/functions/FunctionMisc.java:
##########
@@ -113,7 +155,9 @@ void currentUser(HplsqlParser.Expr_spec_funcContext ctx) {
}
public static Var currentUser() {
- return new Var("CURRENT_USER()");
+ String currentUser = System.getProperty("user.name");
+ currentUser = Utils.quoteString(currentUser);
+ return new Var(currentUser);
Review Comment:
Why do we need quotes here? The variable has string type:
https://github.com/apache/hive/blob/be857bf83c00e41a315a7cc8e013832cf169ef18/hplsql/src/main/java/org/apache/hive/hplsql/Var.java#L77-L80
##########
hplsql/src/main/java/org/apache/hive/hplsql/Var.java:
##########
@@ -612,7 +613,7 @@ else if (type == Type.TIMESTAMP) {
if (t.length() > len) {
t = t.substring(0, len);
}
- return String.format("TIMESTAMP '%s'", t);
+ return t;
Review Comment:
Is it safe to remove the `TIMESTAMP`?
A few lines above a comment says the value can be `.0`. Is it a float or a
timestamp?
https://github.com/apache/hive/blob/be857bf83c00e41a315a7cc8e013832cf169ef18/hplsql/src/main/java/org/apache/hive/hplsql/Var.java#L608C54-L608C56
--
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]
