[GitHub] spark pull request: [SPARK-9460] Fix prefix generation for UTF8Str...

2015-07-30 Thread rednaxelafx
Github user rednaxelafx commented on the pull request: https://github.com/apache/spark/pull/7789#issuecomment-126214413 Yes, this change looks good to me, for a semantically safe bet on the object layout. --- If your project is set up for it, you can reply to this email and have

[GitHub] spark pull request #17777: [SPARK-20482][SQL] Resolving Casts is too strict ...

2017-04-26 Thread rednaxelafx
GitHub user rednaxelafx opened a pull request: https://github.com/apache/spark/pull/1 [SPARK-20482][SQL] Resolving Casts is too strict on having time zone set ## What changes were proposed in this pull request? Relax the requirement that a `TimeZoneAwareExpression` has

[GitHub] spark pull request #17777: [SPARK-20482][SQL] Resolving Casts is too strict ...

2017-04-26 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/1#discussion_r113589540 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala --- @@ -165,6 +165,22 @@ case class Cast(child: Expression

[GitHub] spark pull request #17777: [SPARK-20482][SQL] Resolving Casts is too strict ...

2017-04-26 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/1#discussion_r113589853 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala --- @@ -165,6 +165,22 @@ case class Cast(child: Expression

[GitHub] spark issue #17777: [SPARK-20482][SQL] Resolving Casts is too strict on havi...

2017-04-26 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/1 I'll update again to address comments from @gatorsmile --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does

[GitHub] spark pull request #17777: [SPARK-20482][SQL] Resolving Casts is too strict ...

2017-04-27 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/1#discussion_r113751468 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala --- @@ -165,6 +181,14 @@ case class Cast(child: Expression

[GitHub] spark pull request #17777: [SPARK-20482][SQL] Resolving Casts is too strict ...

2017-04-27 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/1#discussion_r113751399 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala --- @@ -89,6 +89,22 @@ object Cast { case

[GitHub] spark issue #17777: [SPARK-20482][SQL] Resolving Casts is too strict on havi...

2017-04-27 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/1 Thanks for the review! I'll update the patch again. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] spark pull request #18255: [SPARK-21038][SQL] Reduce redundant generated ini...

2017-06-09 Thread rednaxelafx
Github user rednaxelafx closed the pull request at: https://github.com/apache/spark/pull/18255 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] spark pull request #15227: [SPARK-17655][SQL]Remove unused variables declara...

2017-06-20 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/15227#discussion_r123054373 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala --- @@ -129,11 +131,17 @@ class

[GitHub] spark pull request #18095: [SPARK-20872][SQL] ShuffleExchange.nodeName shoul...

2017-05-24 Thread rednaxelafx
GitHub user rednaxelafx opened a pull request: https://github.com/apache/spark/pull/18095 [SPARK-20872][SQL] ShuffleExchange.nodeName should handle null coordinator ## What changes were proposed in this pull request? A one-liner change in `ShuffleExchange.nodeName` to cover

[GitHub] spark pull request #18095: [SPARK-20872][SQL] ShuffleExchange.nodeName shoul...

2017-05-24 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/18095#discussion_r118382726 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/ShuffleExchange.scala --- @@ -40,14 +40,17 @@ case class ShuffleExchange

[GitHub] spark issue #18257: [SPARK-21044][SPARK-21041][SQL] Add RemoveInvalidRange o...

2017-06-09 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/18257 The end result looks good to me. Thanks for fixing it! Although, I'd prefer fixing the actual integer overflow handling in `RangeExec`'s codegen, too. Even though your fix will handle

[GitHub] spark pull request #18255: [SPARK-21038][SQL] Reduce redundant generated ini...

2017-06-09 Thread rednaxelafx
GitHub user rednaxelafx opened a pull request: https://github.com/apache/spark/pull/18255 [SPARK-21038][SQL] Reduce redundant generated init code in Catalyst codegen ## What changes were proposed in this pull request? In Java, instance fields are guaranteed to be first

[GitHub] spark pull request #18095: [SPARK-20872][SQL] ShuffleExchange.nodeName shoul...

2017-05-24 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/18095#discussion_r118359189 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/ShuffleExchange.scala --- @@ -47,7 +47,7 @@ case class ShuffleExchange

[GitHub] spark issue #18095: [SPARK-20872][SQL] ShuffleExchange.nodeName should handl...

2017-05-24 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/18095 Updated patch to address the comments on making two other match conditions on `coordinator` so that they're consistently handled. --- If your project is set up for it, you can reply

[GitHub] spark pull request #19488: [SPARK-22266][SQL] The same aggregate function wa...

2017-10-13 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19488#discussion_r144651235 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala --- @@ -205,14 +205,17 @@ object PhysicalAggregation

[GitHub] spark pull request #19083: [SPARK-21871][SQL] Check actual bytecode size whe...

2017-08-30 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19083#discussion_r135988601 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala --- @@ -1091,6 +,7 @@ object

[GitHub] spark pull request #19083: [SPARK-21871][SQL] Check actual bytecode size whe...

2017-08-30 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19083#discussion_r135985618 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala --- @@ -1001,6 +1001,16 @@ abstract class

[GitHub] spark pull request #19083: [SPARK-21871][SQL] Check actual bytecode size whe...

2017-08-30 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19083#discussion_r135989181 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala --- @@ -176,34 +176,46 @@ class WholeStageCodegenSuite

[GitHub] spark pull request #19083: [SPARK-21871][SQL] Check actual bytecode size whe...

2017-08-30 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19083#discussion_r135996601 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala --- @@ -1001,6 +1001,16 @@ abstract class

[GitHub] spark pull request #19082: [SPARK-21870][SQL] Split aggregation code into sm...

2017-09-01 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19082#discussion_r136506452 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala --- @@ -244,6 +246,92 @@ case class

[GitHub] spark pull request #19082: [SPARK-21870][SQL] Split aggregation code into sm...

2017-09-01 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19082#discussion_r136506046 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala --- @@ -244,6 +246,92 @@ case class

[GitHub] spark pull request #19083: [SPARK-21871][SQL] Check actual bytecode size whe...

2017-08-30 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/19083#discussion_r136250152 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala --- @@ -1001,6 +1001,16 @@ abstract class

[GitHub] spark issue #19890: [SPARK-22688][SQL] Upgrade Janino version to 3.0.8

2017-12-06 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/19890 I'd LGTM on both upgrading to Janino 3.0.8 and also changing the exception type captured from `JaninoRuntimeException` to the new `InternalCompilerException`. It's never a good idea to have

[GitHub] spark pull request #21193: [SPARK-24121][SQL] Add API for handling expressio...

2018-05-18 Thread rednaxelafx
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] spark pull request #21193: [SPARK-24121][SQL] Add API for handling expressio...

2018-05-18 Thread rednaxelafx
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] spark pull request #21193: [SPARK-24121][SQL] Add API for handling expressio...

2018-05-18 Thread rednaxelafx
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] spark issue #21106: [SPARK-23711][SQL] Add fallback generator for UnsafeProj...

2018-05-18 Thread rednaxelafx
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] spark pull request #21367: [SPARK-24321][SQL] Extract common code from Divid...

2018-05-21 Thread rednaxelafx
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] spark pull request #21367: [SPARK-24321][SQL] Extract common code from Divid...

2018-05-18 Thread rednaxelafx
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] spark pull request #21193: [SPARK-24121][SQL] Add API for handling expressio...

2018-05-22 Thread rednaxelafx
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] spark pull request #21193: [SPARK-24121][SQL] Add API for handling expressio...

2018-05-22 Thread rednaxelafx
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] spark issue #21367: [SPARK-24321][SQL] Extract common code from Divide/Remai...

2018-05-22 Thread rednaxelafx
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] spark pull request #21367: [SPARK-24321][SQL] Extract common code from Divid...

2018-05-18 Thread rednaxelafx
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] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...

2018-06-26 Thread rednaxelafx
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] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...

2018-06-26 Thread rednaxelafx
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] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...

2018-06-26 Thread rednaxelafx
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] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...

2018-06-26 Thread rednaxelafx
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] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...

2018-06-26 Thread rednaxelafx
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] spark pull request #20163: [SPARK-22966][PySpark] Spark SQL should handle Py...

2018-01-04 Thread rednaxelafx
GitHub user rednaxelafx opened a pull request: https://github.com/apache/spark/pull/20163 [SPARK-22966][PySpark] Spark SQL should handle Python UDFs that return a datetime.date or datetime.datetime ## What changes were proposed in this pull request? Perform appropriate

[GitHub] spark pull request #20158: Fix typo in comments in PySpark's udf() definitio...

2018-01-04 Thread rednaxelafx
GitHub user rednaxelafx opened a pull request: https://github.com/apache/spark/pull/20158 Fix typo in comments in PySpark's udf() definition ## What changes were proposed in this pull request? Just a simple typo fix from `curcuiting` to `circuiting`. ## How

[GitHub] spark issue #20158: [PySpark] Fix typo in comments in PySpark's udf() defini...

2018-01-04 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20158 @srowen Thanks for your review! I'll have a look at other typos and update this PR later. I was actually working on code around this place and thought I'd split out the typo fix

[GitHub] spark issue #20163: [SPARK-22966][PySpark] Spark SQL should handle Python UD...

2018-01-05 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20163 Thanks for all of your comments, @HyukjinKwon and @icexelloss ! I'd like to wait for more discussions / suggestions on whether or not we want a behavior change that makes this reproducer

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-10 Thread rednaxelafx
GitHub user rednaxelafx opened a pull request: https://github.com/apache/spark/pull/20224 [SPARK-23032][SQL] Add a per-query codegenStageId to WholeStageCodegenExec ## What changes were proposed in this pull request? **Proposal** Add a per-query ID to the codegen

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-10 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 jenkins retest this please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-10 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 One comment as to using `ThreadLocal[Integer]` for keeping track of the IDs: I did have an alternative implementation of this PR that declares `WholeStageCodegenExec` as: ```scala case

[GitHub] spark issue #20163: [SPARK-22966][PySpark] Spark SQL should handle Python UD...

2018-01-10 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20163 Given the above discussion, do we have consensus on all of the following: - Update the documentation for PySpark UDFs to warn about the behavior of mismatched declared `returnType` vs actual

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-23 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 jenkins retest this please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-22 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 I've updated the PR addressing @gatorsmile 's comments: moved the new utility code to `WholeStageCodegenId` object and added a new test case in `HiveExplainSuite`. ping @gatorsmile

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-25 Thread rednaxelafx
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

[GitHub] spark issue #20158: [PySpark] Fix typo in comments in PySpark's udf() defini...

2018-01-17 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20158 @HyukjinKwon cool! Thanks a lot for including it! Closing this one now. --- - To unsubscribe, e-mail: reviews-unsubscr

[GitHub] spark pull request #20158: [PySpark] Fix typo in comments in PySpark's udf()...

2018-01-17 Thread rednaxelafx
Github user rednaxelafx closed the pull request at: https://github.com/apache/spark/pull/20158 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-17 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r161979484 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala --- @@ -312,6 +313,24 @@ case class InputAdapter(child

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-17 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 Thanks @gatorsmile ! Will add a new test case in `HiveExplainSuite`. --- - To unsubscribe, e-mail: reviews-unsubscr

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-17 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r161980740 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala --- @@ -575,7 +609,10 @@ case class CollapseCodegenStages

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-24 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163757952 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala --- @@ -325,6 +326,28 @@ object WholeStageCodegenExec

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-24 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163757687 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala --- @@ -629,6 +629,13 @@ object SQLConf { .booleanConf

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163771262 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala --- @@ -228,4 +229,35 @@ class WholeStageCodegenSuite

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-24 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 Updated the PR: 1. addressed @cloud-fan 's comment to make sure the `codegenStageId` is properly copied in transformations after `CollapseCodegenStages`. Added a new unit test case

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163947756 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala --- @@ -471,19 +531,21 @@ case class

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 Updated again. Addressed @viirya 's comments: 1. added comments to explain where this codegen stage ID is used 2. moved an assertion message to a comment

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163784607 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala --- @@ -228,4 +229,35 @@ class WholeStageCodegenSuite

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163784286 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala --- @@ -325,6 +326,32 @@ object WholeStageCodegenExec

[GitHub] spark pull request #20224: [SPARK-23032][SQL] Add a per-query codegenStageId...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163799937 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala --- @@ -228,4 +229,38 @@ class WholeStageCodegenSuite

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-25 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 Updated again to address @cloud-fan 's comments: removed unneeded test case and added a bit more comments

[GitHub] spark pull request #20163: [SPARK-22966][PYTHON][SQL] Python UDFs with retur...

2018-01-12 Thread rednaxelafx
Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20163#discussion_r161287717 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/python/EvaluatePython.scala --- @@ -144,6 +145,7 @@ object EvaluatePython

[GitHub] spark issue #20247: [SPARK-23021][SQL] AnalysisBarrier should override inner...

2018-01-12 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20247 Thanks for fixing it! One thing I'm curious what others think is, whether or not the `AnalysisBarrier` nodes themselves should show up in the explain output at all, i.e. should

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-11 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 Thanks for your comments and questions, @kiszk and @maropu ! Let me address them in a couple of separate points. **tl;dr** On top of my original proposal in the PR

[GitHub] spark issue #20224: [SPARK-23032][SQL] Add a per-query codegenStageId to Who...

2018-01-11 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20224 Thanks for your comments, @viirya ! I'd say only having (1) and (2) makes it much less useful than having all 3, but it's still useful in its own for helping people understand exactly

[GitHub] spark issue #20163: [SPARK-22966][PYTHON][SQL] Python UDFs with returnType=S...

2018-01-11 Thread rednaxelafx
Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20163 jenkins retest this please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e

[GitHub] spark issue #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comme...

2018-01-29 Thread rednaxelafx
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] spark issue #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comme...

2018-01-30 Thread rednaxelafx
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] spark issue #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comme...

2018-01-28 Thread rednaxelafx
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] spark issue #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comme...

2018-02-01 Thread rednaxelafx
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] spark issue #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comme...

2018-01-31 Thread rednaxelafx
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] spark issue #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comme...

2018-01-31 Thread rednaxelafx
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] spark pull request #20434: [SPARK-23267] [SQL] Increase spark.sql.codegen.hu...

2018-01-30 Thread rednaxelafx
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] spark pull request #20419: [SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId i...

2018-02-12 Thread rednaxelafx
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] spark pull request #20599: [SPARK-23407][SQL] add a config to try to inline ...

2018-02-14 Thread rednaxelafx
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] spark issue #20626: [SPARK-23447][SQL] Cleanup codegen template for Literal

2018-02-16 Thread rednaxelafx
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] spark pull request #20626: [SPARK-23447][SQL] Cleanup codegen template for L...

2018-02-15 Thread rednaxelafx
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] spark issue #20626: [SPARK-23447][SQL] Cleanup codegen template for Literal

2018-02-16 Thread rednaxelafx
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] spark issue #20626: [SPARK-23447][SQL] Cleanup codegen template for Literal

2018-02-16 Thread rednaxelafx
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] spark issue #20626: [SPARK-23447][SQL] Cleanup codegen template for Literal

2018-02-16 Thread rednaxelafx
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] spark issue #20626: [SPARK-23447][SQL] Cleanup codegen template for Literal

2018-02-16 Thread rednaxelafx
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] spark issue #20043: [SPARK-22856][SQL] Add wrappers for codegen output and n...

2018-02-16 Thread rednaxelafx
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] spark issue #20612: [SPARK-23424][SQL]Add codegenStageId in comment

2018-02-15 Thread rednaxelafx
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] spark issue #20645: SPARK-23472: Add defaultJavaOptions for drivers and exec...

2018-02-22 Thread rednaxelafx
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] spark issue #21951: [SPARK-24957][SQL][FOLLOW-UP] Clean the code for AVERAGE

2018-08-01 Thread rednaxelafx
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] spark pull request #22103: [SPARK-25113][SQL] Add logging to CodeGenerator w...

2018-08-14 Thread rednaxelafx
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] spark pull request #22103: [SPARK-25113][SQL] Add logging to CodeGenerator w...

2018-08-14 Thread rednaxelafx
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] spark pull request #22103: [SPARK-25113][SQL] Add logging to CodeGenerator w...

2018-08-14 Thread rednaxelafx
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] spark pull request #22103: [SPARK-25113][SQL] Add logging to CodeGenerator w...

2018-08-14 Thread rednaxelafx
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] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...

2018-08-21 Thread rednaxelafx
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] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...

2018-08-21 Thread rednaxelafx
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] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...

2018-08-21 Thread rednaxelafx
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] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...

2018-08-21 Thread rednaxelafx
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] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...

2018-08-21 Thread rednaxelafx
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] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...

2018-08-21 Thread rednaxelafx
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

  1   2   >