Skip to content

Conversation

@dilipbiswal
Copy link
Contributor

What changes were proposed in this pull request?

Sorting array of booleans (not nullable) returns a compilation error in the code generation phase. Below is the compilation error :

java.util.concurrent.ExecutionException: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 51, Column 23: failed to compile: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 51, Column 23: No applicable constructor/method found for actual parameters "boolean[]"; candidates are: "public static void java.util.Arrays.sort(long[])", "public static void java.util.Arrays.sort(long[], int, int)", "public static void java.util.Arrays.sort(byte[], int, int)", "public static void java.util.Arrays.sort(float[])", "public static void java.util.Arrays.sort(float[], int, int)", "public static void java.util.Arrays.sort(char[])", "public static void java.util.Arrays.sort(char[], int, int)", "public static void java.util.Arrays.sort(short[], int, int)", "public static void java.util.Arrays.sort(short[])", "public static void java.util.Arrays.sort(byte[])", "public static void java.util.Arrays.sort(java.lang.Object[], int, int, java.util.Comparator)", "public static void java.util.Arrays.sort(java.lang.Object[], java.util.Comparator)", "public static void java.util.Arrays.sort(int[])", "public static void java.util.Arrays.sort(java.lang.Object[], int, int)", "public static void java.util.Arrays.sort(java.lang.Object[])", "public static void java.util.Arrays.sort(double[])", "public static void java.util.Arrays.sort(double[], int, int)", "public static void java.util.Arrays.sort(int[], int, int)"
	at com.google.common.util.concurrent.AbstractFuture$Sync.getValue(AbstractFuture.java:306)
	at com.google.common.util.concurrent.AbstractFuture$Sync.get(AbstractFuture.java:293)
	at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:116)
	at com.google.common.util.concurrent.Uninterruptibles.getUninterruptibly(Uninterruptibles.java:135)
	at com.google.common.cache.LocalCache$Segment.getAndRecordStats(LocalCache.java:2410)
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2380)
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2342)
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2257)
	at com.google.common.cache.LocalCache.get(LocalCache.java:4000)
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4004)
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4874)
	at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$.compile(CodeGenerator.scala:1305)

How was this patch tested?

Added test in collectionExpressionSuite

@kiszk
Copy link
Member

kiszk commented Sep 2, 2018

Good catch, LGTM

@dilipbiswal dilipbiswal changed the title [SPARK-25307] ArraySort function may return an error in the code generation phase [SPARK-25307][SQL] ArraySort function may return an error in the code generation phase Sep 2, 2018
@dilipbiswal
Copy link
Contributor Author

@kiszk Thanks. Can you please help look at 22315 as well.

@SparkQA
Copy link

SparkQA commented Sep 2, 2018

Test build #95585 has finished for PR 22314 at commit 7dec581.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dilipbiswal
Copy link
Contributor Author

retest this please

@ueshin
Copy link
Member

ueshin commented Sep 2, 2018

LGTM.

@SparkQA
Copy link

SparkQA commented Sep 2, 2018

Test build #95588 has finished for PR 22314 at commit 7dec581.

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

Copy link
Member

Choose a reason for hiding this comment

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

nonNullPrimitiveNumericAscendingSort or fastSortCode preferred?

Copy link
Contributor Author

@dilipbiswal dilipbiswal Sep 3, 2018

Choose a reason for hiding this comment

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

@maropu Lets keep the variable name the same. I personally find the name more descriptive. Lets add a private def like you suggest in the following comment.

Copy link
Member

@maropu maropu Sep 3, 2018

Choose a reason for hiding this comment

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

How about making this condition a separate private method like isFastSortSupported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will make a change

Copy link
Member

@maropu maropu Sep 3, 2018

Choose a reason for hiding this comment

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

sorry, but not method but variable is ok (I said method in the previous comment though...), e.g.,

val canPerformFastSort = {
  CodeGenerator.isPrimitiveType(elementType) && elementType != BooleanType && !containsNull
}

@SparkQA
Copy link

SparkQA commented Sep 3, 2018

Test build #95603 has finished for PR 22314 at commit 2f1b192.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 3, 2018

Test build #95601 has finished for PR 22314 at commit 2e893ad.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dilipbiswal
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Sep 3, 2018

Test build #95604 has finished for PR 22314 at commit 2f1b192.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 3, 2018

Test build #95617 has finished for PR 22314 at commit 3645438.

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

@SparkQA
Copy link

SparkQA commented Sep 3, 2018

Test build #95635 has finished for PR 22314 at commit d27256e.

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

@ueshin
Copy link
Member

ueshin commented Sep 4, 2018

Thanks! merging to master.

@asfgit asfgit closed this in b60ee3a Sep 4, 2018
@dilipbiswal
Copy link
Contributor Author

@ueshin @kiszk @maropu Thanks a lot.

@ueshin
Copy link
Member

ueshin commented Sep 4, 2018

@dilipbiswal Do we need to backport this to 2.3? If so, could you submit a backport pr to branch-2.3 please? Thanks!

@dilipbiswal
Copy link
Contributor Author

@ueshin Sure.

@dilipbiswal
Copy link
Contributor Author

@ueshin We don't have codegen enabled for this function in 2.3. So thankfully we don't have this problem :-)

@ueshin
Copy link
Member

ueshin commented Sep 4, 2018

@dilipbiswal Thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants