Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/23079#discussion_r234534798
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicateSuite.scala
---
@@ -298,6 +299,45
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/23079#discussion_r234508866
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicateSuite.scala
---
@@ -298,6 +299,45
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/23079#discussion_r234508561
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
---
@@ -767,6 +767,15 @@ object ReplaceNullWithFalse
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/23079
cc @aokolnychyi : I'd like to propose renaming the rule you introduced to
add a `-InPredicate` suffix, because obviously we can't replace arbitrary
`null`s with `false`, but only the ones
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/23079
[SPARK-26107][SQL] Extend ReplaceNullWithFalseInPredicate to support
higher-order functions: ArrayExists, ArrayFilter, MapFilter
## What changes were proposed in this pull request
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r234385204
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -17,17 +17,33
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22976#discussion_r233771269
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
---
@@ -68,62 +68,55 @@ object
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22976
+1 LGTM, thanks for making this improvement!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/23032
Ha, it's failing an Avro test case due to:
```
Cause: org.codehaus.commons.compiler.CompileException: File
'generated.java', Line 65, Column 33: failed to compile
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/23032
cc @viirya @maropu @kiszk
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/23032
[SPARK-X][SQL][MINOR] Reduce the number of unused UnsafeRowWriters
created in whole-stage codegen
## What changes were proposed in this pull request?
Reduce the number of unused
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22847
Just in case people wonder, the following is the hack patch that I used for
stress testing code splitting before this PR:
```diff
---
a/sql/catalyst/src/main/scala/org/apache/spark/sql
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22847#discussion_r229943260
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -812,6 +812,17 @@ object SQLConf {
.intConf
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22847#discussion_r229942325
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -812,6 +812,17 @@ object SQLConf {
.intConf
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22847
Can / should we apply the same threshold conf to
`Expression.reduceCodeSize()`?
cc @yucai @cloud-fan @kiszk
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22847
Using source code length will have to be a very coarse-grained, "fuzzy"
heuristic. It's not meant to be accurate. So just pick some number that makes
sense.
2048 might be a g
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/22506
[SPARK-25494][SQL] Upgrade Spark's use of Janino to 3.0.10
## What changes were proposed in this pull request?
This PR upgrades Spark's use of Janino from 3.0.9 to 3.0.10.
Note
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22355
LGTM as well.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22429#discussion_r218235962
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala ---
@@ -189,23 +192,32 @@ class QueryExecution(val sparkSession
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22355#discussion_r217016675
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedMutableProjection.scala
---
@@ -0,0 +1,83
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22355#discussion_r216525084
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedMutableProjection.scala
---
@@ -0,0 +1,83
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22355#discussion_r216524666
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala
---
@@ -86,24 +86,12 @@ package object expressions
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22355#discussion_r216526434
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedMutableProjection.scala
---
@@ -0,0 +1,83
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22203
Thanks @srowen ! In that case could we add a few mentions of such test
cases that failed into the PR description itself, perhaps a few lines under the
"How was this patch tested?&quo
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22203
I'm wondering, though, either in the same PR or in a separate PR, could we
add a test case in Spark that would trigger the "two non-abstract method" error
so that we'd show an exampl
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22203
LGTM
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/22187
So the new solution now is to directly ship the `StructType` object as a
reference object. Why not? ;-)
I'm +1 on shipping the object directly instead of generating code to
recreate
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211731921
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -17,24 +17,10
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211731624
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211731392
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211487231
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -40,4 +55,13
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211484515
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211485732
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -40,4 +55,13
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211485314
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -40,4 +55,13
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211485848
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211484616
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211484374
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -18,12 +18,27
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22103#discussion_r210169785
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
---
@@ -1385,9 +1386,15 @@ object
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22103#discussion_r210130579
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
---
@@ -1385,9 +1386,15 @@ object
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22103#discussion_r210129972
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
---
@@ -1385,9 +1386,15 @@ object
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/22103
[SPARK-25113][SQL] Add logging to CodeGenerator when any generated method's
bytecode size goes above HugeMethodLimit
## What changes were proposed in this pull request?
Add logging
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21951
LGTM as well. Thanks a lot!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21699
This mostly looks good, but I'd like to ask a few things first:
1. The new overloaded `pivot()` that takes `Column` only exist for
`pivot(Column, Seq[Any])`. Were you planning to add
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198370477
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala
---
@@ -104,4 +104,40 @@ class ComplexDataSuite
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198268691
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala
---
@@ -104,4 +104,38 @@ class ComplexDataSuite
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21643
Thanks for your comments, @MaxGekk @maropu @cloud-fan , I've tweaked the
unit test case to address your comments. Please check it out again
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21643
https://github.com/apache/spark/pull/21643#pullrequestreview-131977512
For all practical purposes, no, this change shouldn't break any use cases
that we support; there could be use cases
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/21643
[SPARK-24659][SQL] GenericArrayData.equals should respect element type
differences
## What changes were proposed in this pull request?
Fix `GenericArrayData.equals`, so
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21367
Updated PR, addressed @cloud-fan and @viirya 's comments.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21193#discussion_r189785968
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeBlockSuite.scala
---
@@ -23,12 +23,20 @@ import
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21193#discussion_r189786155
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
---
@@ -167,9 +170,40 @@ object Block
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21367#discussion_r189723394
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -297,13 +279,39 @@ case class Divide(left
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21367#discussion_r189404152
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -220,30 +220,12 @@ case class Multiply(left
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/21367
[SPARK-24321][SQL] Extract common code from Divide/Remainder to a base trait
## What changes were proposed in this pull request?
Extract common code from `Divide`/`Remainder` to a new
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21106
Should this PR wait for https://github.com/apache/spark/pull/21299 so that
accessing confs on executors can be made simpler
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21193#discussion_r189229566
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeBlockSuite.scala
---
@@ -0,0 +1,130
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21193#discussion_r189225184
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
---
@@ -114,6 +114,115 @@ object JavaCode
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21193#discussion_r189229229
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeBlockSuite.scala
---
@@ -0,0 +1,130
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21094
LGTM
Wow, that's a hideous bug...
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20772
Just wanna leave a note here that the Janino side of the bug has been fixed:
https://github.com/janino-compiler/janino/pull/46#issuecomment-380799160
https://github.com/janino-compiler
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21039
I just checked the same test in Build 4695, which still has this change,
and the test passed:
https://amplab.cs.berkeley.edu/jenkins/job/spark-master-test-maven-hadoop-2.7/4695/testReport
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21039
Thanks for reverting it for me. The test failure was definitely related to
the explicit nulling from this PR, but I can't see how that's possible yet.
First of all, in the build
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21039#discussion_r180721843
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala
---
@@ -236,6 +236,8 @@ case class
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/21039
[SPARK-23960][SQL][MINOR] Mark HashAggregateExec.bufVars as transient
## What changes were proposed in this pull request?
Mark `HashAggregateExec.bufVars` as transient to avoid it from
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21026#discussion_r180598651
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala
---
@@ -352,12 +346,10 @@ case class IsNotNull
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21026#discussion_r180575894
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala
---
@@ -107,7 +110,7 @@ object
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21026#discussion_r180583517
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
---
@@ -0,0 +1,145 @@
+/*
+ * Licensed
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21026#discussion_r180575717
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala
---
@@ -292,8 +292,7 @@ object
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21026#discussion_r180575964
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala
---
@@ -76,7 +78,7 @@ object
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21026#discussion_r180575803
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala
---
@@ -128,7 +131,7 @@ object
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/21016
[SPARK-23947][SQL] Add hashUTF8String convenience method to hasher classes
## What changes were proposed in this pull request?
Add `hashUTF8String()` to the hasher classes to allow
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/19082#discussion_r179367236
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala
---
@@ -257,6 +259,78 @@ case class
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20779
Sorry for the late comment. This PR itself is LGTM. I'd just like to make
some side comments on why this bug is happening.
Janino uses a peculiar encoding of implementing bridge methods
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20870
jenkins retest this please
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20870#discussion_r175986986
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
---
@@ -942,7 +940,7 @@ class
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/20870
[SPARK-23760][SQL]: CodegenContext.withSubExprEliminationExprs should
save/restore CSE state correctly
## What changes were proposed in this pull request?
Fixed
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20753
@hvanhovell @kiszk Yes, the old school JDK reflection does perform bytecode
generation to speed it up, since the JDK1.4 era. But the advantage of fully
optimized `MethodHandle` versus
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20753#discussion_r173075682
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -133,8 +134,21 @@ case class StaticInvoke
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20757#discussion_r173010942
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1408,11 +1409,37 @@ case class
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20757#discussion_r173006281
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
---
@@ -1408,11 +1409,37 @@ case class
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20700
Aha! Thanks @kiszk san for working on this! I really wanted the stateless
methods to be extracted so that I can use more utils without having to pass
around a `CodegenContext` for no good
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20645
A quick question: after this change, `extraJavaOptions` is still able to
cleanly override whatever's set in `defaultJavaOptions`, is that right?
^^ Just making sure I understood
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20043
Hi @viirya, this PR is definitely onto the right direction. I'd like to
review this PR as well and hopeful help polish it in for post-2.3. Thanks for
your work
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20626
@hvanhovell Thanks a lot! You're absolutely right. Update the PR
accordingly. It's passing tests in my local testing and hopefully it'll pass
the Jenkins tests as well
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20626
Ah...I see, there are more places where they're statically referencing some
variable but dynamically those variables would always be null. I'll update the
PR later to fix those places as well
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20626
cc @cloud-fan @hvanhovell
Note: this is for master and branch-2.3 post 2.3.0 release.
---
-
To unsubscribe, e-mail
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20626
So I was able to find quite a few cases where the `DUMMY` placeholder
caught uses of the `value` field outside of appropriate null-checked regions.
I'll check the individual cases
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20626
jenkins retest this please
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/20626
[SPARK-23447][SQL] Cleanup codegen template for Literal
## What changes were proposed in this pull request?
Cleaned up the codegen templates for `Literal`s, to make sure
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20612
LGTM. Let's wait for one more LGTM from @gatorsmile / @cloud-fan .
---
-
To unsubscribe, e-mail: reviews-unsubscr
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20599#discussion_r168101849
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -675,6 +675,16 @@ object SQLConf {
"disable lo
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20419#discussion_r167775177
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
---
@@ -1226,14 +1226,21 @@ class
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20419
@kiszk For this specific kind of usage, I don't think using a hardcoded
stable ID will be a problem.
The comment we're talking about is the kind the can only appear once in a
single whole
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20419
I gave it a bit more thought. Here's an alternative proposal: instead of
using a "force comment" mechanism in the current form (which still gets a
`ctx.freshName("c")`
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20419
I did the experiment that I asked about at
https://github.com/apache/spark/pull/20419#issuecomment-361359825 , and
verified that under the current implementation, this PR will not affect
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20419
@gatorsmile Not directly. The `CodeAndComment` case class is just a
"container", it doesn't handle what gets into the `body` field. When we force
embed a comment, it'll leave
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20434#discussion_r164687283
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -660,12 +660,10 @@ object SQLConf {
val
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20419
@kiszk SGTM and LGTM. Let's ship it!
One more question on the side: with the `forceComment = true`, are we fully
sure that won't affect the equality of `CodeAndComment`?
The whole
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/20419
LGTM, and +1 on @viirya 's idea. I like it better for the comment to be on
top of the class declaration instead of inside it; but I'm okay either way if
others have strong opinion otherwise
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/20224#discussion_r164019336
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala
---
@@ -471,19 +531,21 @@ case class
1 - 100 of 159 matches
Mail list logo
