Skip to content
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

Jump to bottom

[SPARK-26502][SQL] Move hiveResultString() from QueryExecution to HiveResult #23409

Closed
wants to merge 8 commits into from
Closed

[SPARK-26502][SQL] Move hiveResultString() from QueryExecution to HiveResult #23409

wants to merge 8 commits into from

Conversation

MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Dec 29, 2018

What changes were proposed in this pull request?

In the PR, I propose to move hiveResultString() out of QueryExecution and put it to a separate object.

@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 29, 2018

@hvanhovell I tried to move it to a test class but hiveResultString is used SparkSQLDriver in hive-thriftserver. Having a dependency of a test module in hive-thriftserver doesn't look nice. It cannot be moved out of sql otherwise we get a cycle in dependencies. WDYT?

@SparkQA
Copy link

SparkQA commented Dec 29, 2018

Test build #100538 has finished for PR 23409 at commit bb045dd.

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

@dongjoon-hyun
Copy link
Member

@MaxGekk . Could you revise the PR title a little bit? Move or Refactor would be better. Or, maybe Create HiveResult ...?

@SparkQA
Copy link

SparkQA commented Dec 30, 2018

Test build #100543 has finished for PR 23409 at commit 1c1b4df.

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

@MaxGekk MaxGekk changed the title [SPARK-26502][SQL] Get rid of hiveResultString() in QueryExecution [SPARK-26502][SQL] Move hiveResultString() from QueryExecution to HiveResult Dec 30, 2018
hvanhovell
hvanhovell reviewed Dec 30, 2018
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/HiveResult.scala
import org.apache.spark.sql.execution.command.{DescribeTableCommand, ExecutedCommandExec, ShowTablesCommand}
import org.apache.spark.sql.types._

object HiveResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc?

Copy link
Contributor

Choose a reason for hiding this comment

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

HiveResult.hiveResultString seems could used for other(eg. MySql).So I suggest modify the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be used from different places. The main purpose of these methods is to return results in Hive-like form.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, let's stick with HiveResult.

@SparkQA
Copy link

SparkQA commented Jan 1, 2019

Test build #100618 has finished for PR 23409 at commit 948414a.

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

@SparkQA
Copy link

SparkQA commented Jan 1, 2019

Test build #100616 has finished for PR 23409 at commit 7aba95b.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@MaxGekk
Copy link
Member Author

MaxGekk commented Jan 1, 2019

@hvanhovell The last commit 948414a can be merged.

@MaxGekk
Copy link
Member Author

MaxGekk commented Jan 2, 2019

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented Jan 3, 2019

Test build #100660 has finished for PR 23409 at commit 948414a.

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

HyukjinKwon
HyukjinKwon reviewed Jan 3, 2019
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/HiveResult.scala
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._

/**
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it's just moving Hive stuff from sql to sql. If it were getting rid of it completely or moving it to test scope only, then I'm supportive but now it just move the function.

Kind of neutral on this. Code itself looks good. If any committer pushes it within few days, then it's good. Otherwise, I would close.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it's just moving Hive stuff from sql to sql.

This is not just moving it inside of sql. The idea is to clean up common class QueryExecution from pretty specific method which is used in tests mostly (and in SparkSQLDriver unfortunately).

If it were getting rid of it completely or moving it to test scope only, then I'm supportive but now it just move the function.

I tried all those variants like:

  1. Moving to sql/core/test but it seems weird that hive-thriftserver (SparkSQLDriver) depends on a test jar.
  2. Moving to hive since hive's tests use the method makes cycle dependencies hive <-> sql/core.

It would be nice if SQLQueryTestSuite doesn't depend on Hive related method. Is it really needed?

Otherwise, I would close.

@hvanhovell Please, protect ;-)

hvanhovell
hvanhovell approved these changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

@asfgit asfgit closed this in 2a30deb Jan 3, 2019
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@MaxGekk @jackylee-ch
[SPARK-26502][SQL] Move hiveResultString() from QueryExecution to Hiv…
5819268 
…eResult

## What changes were proposed in this pull request?

In the PR, I propose to move `hiveResultString()` out of `QueryExecution` and put it to a separate object.

Closes apache#23409 from MaxGekk/hive-result-string.

Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Co-authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>
@MaxGekk MaxGekk deleted the hive-result-string branch August 17, 2019 13:36
PavithraRamachandran pushed a commit to PavithraRamachandran/spark that referenced this pull request Sep 24, 2020
@MaxGekk @PavithraRamachandran
[SPARK-26502][SQL] Move hiveResultString() from QueryExecution to Hiv…
c728183 
…eResult

In the PR, I propose to move `hiveResultString()` out of `QueryExecution` and put it to a separate object.

Closes apache#23409 from MaxGekk/hive-result-string.

Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Co-authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@beliefer beliefer beliefer left review comments

@hvanhovell hvanhovell hvanhovell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@MaxGekk @SparkQA @dongjoon-hyun @HyukjinKwon @beliefer @hvanhovell