[MINOR][SQL] Deduplicate huge if statements in get between specialized getters #24016

HeartSaVioR · 2019-03-07T23:02:08Z

What changes were proposed in this pull request?

This patch deduplicates the huge if statements regarding getting value between specialized getters.

How was this patch tested?

Existing UT.

…d getters

HeartSaVioR · 2019-03-08T01:39:44Z

...talyst/src/main/java/org/apache/spark/sql/catalyst/expressions/SpecializedGettersReader.java

+    this.handleUserDefinedType = handleUserDefinedType;
+  }
+
+  public Object read(SpecializedGetters obj, int ordinal, DataType dataType) {


We could even make the class as utility class and no need to make object: just change this to static, and add two boolean parameters. If we prefer not to have object even it's only one for each class, I'll make a change.

I tend to agree, a new object isn't needed.
It could plausibly live in the SpecializedGetters interface in Java 8, but that might be weird.

Yes, I tried to add it to interface via default method, but looked weird so decided to have utility class for this.

SparkQA · 2019-03-08T03:19:56Z

Test build #103178 has finished for PR 24016 at commit f75b497.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class SpecializedGettersReader

SparkQA · 2019-03-08T03:49:35Z

Test build #103180 has finished for PR 24016 at commit 437dd08.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi · 2019-03-11T13:26:33Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeArrayData.java

-    } else {
-      throw new UnsupportedOperationException("Unsupported data type " + dataType.simpleString());
-    }
+    return reader.read(this, ordinal, dataType);


At the first glance it looks odd to pass the this pointer with the same type. Maybe it should be a static function only? Not sure.

That's unavoidable unless we add the method to the interface. I tried it but looked weird.

srowen · 2019-03-11T14:58:30Z

...talyst/src/main/java/org/apache/spark/sql/catalyst/expressions/SpecializedGettersReader.java

+  public Object read(SpecializedGetters obj, int ordinal, DataType dataType) {
+    if (handleNull && (obj.isNullAt(ordinal) || dataType instanceof NullType)) {
+      return null;
+    } else if (dataType instanceof BooleanType) {


Nit: all the 'else's are redundant. It won't make a difference in the bytecode and isn't a big deal, just feels more readable to me if there are going to be 20 of them.

Yeah I imagine this is brought to reduce lines, but given we return every statement, removing else looks OK to me.

srowen · 2019-03-11T14:58:35Z

...talyst/src/main/java/org/apache/spark/sql/catalyst/expressions/SpecializedGettersReader.java

+
+import org.apache.spark.sql.types.*;
+
+public class SpecializedGettersReader {


srowen · 2019-03-11T15:07:54Z

...talyst/src/main/java/org/apache/spark/sql/catalyst/expressions/SpecializedGettersReader.java

+    this.handleUserDefinedType = handleUserDefinedType;
+  }
+
+  public Object read(SpecializedGetters obj, int ordinal, DataType dataType) {


I tend to agree, a new object isn't needed.
It could plausibly live in the SpecializedGetters interface in Java 8, but that might be weird.

srowen · 2019-03-11T15:10:20Z

...talyst/src/main/java/org/apache/spark/sql/catalyst/expressions/SpecializedGettersReader.java

+      return obj.getInt(ordinal);
+    } else if (dataType instanceof TimestampType) {
+      return obj.getLong(ordinal);
+    } else if (dataType instanceof BinaryType) {


The order of evaluating Binary/String/DecimalType varied in the code this replaces. That shouldn't matter right -- none of these types are subtypes of the other.

I wonder, while we're here, is it sensible to put checks for more common types further up? for example, 'short' is pretty rare. String is probably more common than binary or decimal. Those are all maybe more common that date types. Etc. I understand for readability we might not want to move short, but might be worth rearranging the others for some performance.

IMHO, for performance, yes we can move commonly used types to top of huge if statements (though I don't have any statistic for this - we all know string should be placed to one of top), and for readability, this looks to be thoughtfully placed - primitive types first, dates, binary, string, complex types (CalendarInterval could be moved to dates group).

Either is fine for me, and if we want to rearrange to gain performance, we may need to pick some commonly used things according to actual usages. Do you have something to refer?

No, I have nothing but intuition here. I wouldn't make major changes. I think we can probably make the obvious ones, like, String should probably follow primitives.

OK. That's also intuitive to me. Will address. Please let me know if there're other obvious things.

SparkQA · 2019-03-12T01:31:43Z

Test build #103348 has finished for PR 24016 at commit d1f00ac.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public final class SpecializedGettersReader

SparkQA · 2019-03-12T02:10:46Z

Test build #103352 has finished for PR 24016 at commit b7535df.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2019-03-13T20:52:46Z

Merged to master

HeartSaVioR · 2019-03-13T21:29:23Z

Thanks all for reviewing and merging!

HeartSaVioR added 2 commits March 8, 2019 07:57

[MINOR][SQL] Deduplicate huge if statements in get between specialize…

f75b497

…d getters

Fix public -> private

437dd08

HeartSaVioR commented Mar 8, 2019

View reviewed changes

HeartSaVioR mentioned this pull request Mar 11, 2019

[DO-NOT-MERGE][SPARK-27127][SQL] Deduplicate codes reading from/writing to unsafe object #24050

Closed

gaborgsomogyi reviewed Mar 11, 2019

View reviewed changes

srowen reviewed Mar 11, 2019

View reviewed changes

HeartSaVioR added 2 commits March 12, 2019 06:30

Address review comments

d1f00ac

Minor rearrangement

b7535df

HeartSaVioR force-pushed the MINOR-deduplicate-get-from-specialized-getters branch from d049c0f to b7535df Compare March 11, 2019 22:15

srowen approved these changes Mar 12, 2019

View reviewed changes

srowen closed this in 733f2c0 Mar 13, 2019

HeartSaVioR deleted the MINOR-deduplicate-get-from-specialized-getters branch March 13, 2019 21:29


		import org.apache.spark.sql.types.*;

		public class SpecializedGettersReader {

[MINOR][SQL] Deduplicate huge if statements in get between specialized getters #24016

[MINOR][SQL] Deduplicate huge if statements in get between specialized getters #24016

Uh oh!

Conversation

HeartSaVioR commented Mar 7, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 8, 2019

Uh oh!

SparkQA commented Mar 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 12, 2019

Uh oh!

SparkQA commented Mar 12, 2019

Uh oh!

srowen commented Mar 13, 2019

Uh oh!

HeartSaVioR commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HeartSaVioR Mar 11, 2019 •

edited

Loading