Skip to content

[SPARK-40398][CORE][SQL] Use Loop instead of Arrays.stream api #37843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LuciferYang wants to merge 18 commits into from
Closed

[SPARK-40398][CORE][SQL] Use Loop instead of Arrays.stream api #37843

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5c92faa
use loop
LuciferYang Sep 9, 2022
71ff5ff
expression references
LuciferYang Sep 10, 2022
5c5365f
anymatch and all match
LuciferYang Sep 10, 2022
bc19399
sum and avg
LuciferYang Sep 10, 2022
1ecf017
use linked hashset
LuciferYang Sep 11, 2022
9c979c4
to static
LuciferYang Sep 11, 2022
e90991c
imports
LuciferYang Sep 11, 2022
8ff3b77
revert linked hash set
LuciferYang Sep 11, 2022
a16a89b
add comments
LuciferYang Sep 12, 2022
1ee7c4a
add commnets
LuciferYang Sep 12, 2022
46d4a57
change to use HashSet
LuciferYang Sep 13, 2022
a64f608
address comments
LuciferYang Sep 14, 2022
e00330f
address comments
LuciferYang Sep 14, 2022
4892423
fix wrong code
LuciferYang Sep 14, 2022
1a6a3c8
Address srowen's comments
LuciferYang Sep 16, 2022
528f3c5
Merge branch 'upmaster' into ExpressionArrayToStrings
LuciferYang Sep 16, 2022
2343dc8
Merge branch 'upmaster' into ExpressionArrayToStrings
LuciferYang Sep 16, 2022
db94018
Merge branch 'upmaster' into ExpressionArrayToStrings
LuciferYang Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 32 additions & 25 deletions sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@

package org.apache.spark.sql.connector.util;

import java.util.Arrays;
import java.util.ArrayList;
import java.util.List;
import java.util.StringJoiner;
import java.util.stream.Collectors;

import org.apache.spark.sql.connector.expressions.Cast;
import org.apache.spark.sql.connector.expressions.Expression;
Expand Down Expand Up @@ -62,8 +61,7 @@ public String build(Expression expr) {
String name = e.name();
switch (name) {
case "IN": {
List<String> children =
Arrays.stream(e.children()).map(c -> build(c)).collect(Collectors.toList());
List<String> children = expressionsToStringList(e.children());
Copy link
Copy Markdown
Contributor

@mridulm mridulm Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Given this is called only here, by not avoid the subList ? (give start offset and len params to expressionsToStringList)

Copy link
Copy Markdown
Contributor Author

@LuciferYang LuciferYang Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is e00330f ok ?

Copy link
Copy Markdown
Contributor Author

@LuciferYang LuciferYang Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the .subList only wraps a SubList object and does not trigger operations such as memory copy, so the performance gap may be small

Copy link
Copy Markdown
Contributor

@mridulm mridulm Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, sublist is fairly optimal ... but can be avoided here is possible. It is a nit comment actually :-)

return visitIn(children.get(0), children.subList(1, children.size()));
}
case "IS_NULL":
Expand Down Expand Up @@ -159,63 +157,56 @@ public String build(Expression expr) {
case "BIT_LENGTH":
case "CHAR_LENGTH":
case "CONCAT":
return visitSQLFunction(name,
Arrays.stream(e.children()).map(c -> build(c)).toArray(String[]::new));
return visitSQLFunction(name, expressionsToStringArray(e.children()));
case "CASE_WHEN": {
List<String> children =
Arrays.stream(e.children()).map(c -> build(c)).collect(Collectors.toList());
return visitCaseWhen(children.toArray(new String[e.children().length]));
return visitCaseWhen(expressionsToStringArray(e.children()));
}
case "TRIM":
return visitTrim("BOTH",
Arrays.stream(e.children()).map(c -> build(c)).toArray(String[]::new));
return visitTrim("BOTH", expressionsToStringArray(e.children()));
case "LTRIM":
return visitTrim("LEADING",
Arrays.stream(e.children()).map(c -> build(c)).toArray(String[]::new));
return visitTrim("LEADING", expressionsToStringArray(e.children()));
case "RTRIM":
return visitTrim("TRAILING",
Arrays.stream(e.children()).map(c -> build(c)).toArray(String[]::new));
return visitTrim("TRAILING", expressionsToStringArray(e.children()));
case "OVERLAY":
return visitOverlay(
Arrays.stream(e.children()).map(c -> build(c)).toArray(String[]::new));
return visitOverlay(expressionsToStringArray(e.children()));
// TODO supports other expressions
default:
return visitUnexpectedExpr(expr);
}
} else if (expr instanceof Min) {
Min min = (Min) expr;
return visitAggregateFunction("MIN", false,
Arrays.stream(min.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(min.children()));
} else if (expr instanceof Max) {
Max max = (Max) expr;
return visitAggregateFunction("MAX", false,
Arrays.stream(max.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(max.children()));
} else if (expr instanceof Count) {
Count count = (Count) expr;
return visitAggregateFunction("COUNT", count.isDistinct(),
Arrays.stream(count.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(count.children()));
} else if (expr instanceof Sum) {
Sum sum = (Sum) expr;
return visitAggregateFunction("SUM", sum.isDistinct(),
Arrays.stream(sum.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(sum.children()));
} else if (expr instanceof CountStar) {
return visitAggregateFunction("COUNT", false, new String[]{"*"});
} else if (expr instanceof Avg) {
Avg avg = (Avg) expr;
return visitAggregateFunction("AVG", avg.isDistinct(),
Arrays.stream(avg.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(avg.children()));
} else if (expr instanceof GeneralAggregateFunc) {
GeneralAggregateFunc f = (GeneralAggregateFunc) expr;
return visitAggregateFunction(f.name(), f.isDistinct(),
Arrays.stream(f.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(f.children()));
} else if (expr instanceof UserDefinedScalarFunc) {
UserDefinedScalarFunc f = (UserDefinedScalarFunc) expr;
return visitUserDefinedScalarFunction(f.name(), f.canonicalName(),
Arrays.stream(f.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(f.children()));
} else if (expr instanceof UserDefinedAggregateFunc) {
UserDefinedAggregateFunc f = (UserDefinedAggregateFunc) expr;
return visitUserDefinedAggregateFunction(f.name(), f.canonicalName(), f.isDistinct(),
Arrays.stream(f.children()).map(c -> build(c)).toArray(String[]::new));
expressionsToStringArray(f.children()));
} else {
return visitUnexpectedExpr(expr);
}
Expand Down Expand Up @@ -393,4 +384,20 @@ private String joinListToString(
}
return joiner.toString();
}

private String[] expressionsToStringArray(Expression[] expressions) {
String[] result = new String[expressions.length];
for (int i = 0; i < expressions.length; i++) {
result[i] = build(expressions[i]);
}
return result;
}

private List<String> expressionsToStringList(Expression[] expressions) {
List<String> list = new ArrayList<>(expressions.length);
for (Expression expression : expressions) {
list.add(build(expression));
}
return list;
}
}
⚡