Skip to content

Conversation

@aokolnychyi
Copy link
Contributor

What changes were proposed in this pull request?

  • Hard-coded Spark SQL sample snippets were moved into source files under examples sub-project.
  • Removed the inconsistency between Scala and Java Spark SQL examples
  • Scala and Java Spark SQL examples were updated

How was this patch tested?

The work is still in progress. All involved examples were tested manually. An additional round of testing will be done after the code review.

image

@aokolnychyi
Copy link
Contributor Author

@liancheng could you, please, review this PR?

// spark is an existing HiveContext
spark.refreshTable("my_table")
// spark is an existing SparkSession
spark.catalog.refreshTable("my_table")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it the correct way to refresh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@liancheng
Copy link
Contributor

test this please

@liancheng
Copy link
Contributor

add to whitelist

@SparkQA
Copy link

SparkQA commented Jul 11, 2016

Test build #62092 has finished for PR 14119 at commit 95f0f41.

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

@liancheng
Copy link
Contributor

Since you've added JavaSparkSqlExample.scala, we can remove JavaSparkSQL.scala now. (I guess that file was from my original WIP branch?)

@@ -0,0 +1,192 @@
package org.apache.spark.examples.sql;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add Apache license header.

@liancheng
Copy link
Contributor

This looks pretty good! Only found a few minor issues. Thanks for working on it!

@liancheng
Copy link
Contributor

Can we add actual stdout output after each .show() call?

@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Jul 12, 2016

Thanks for the review, I really appreciate. I tried to take into account all comments and update the PR accordingly.

Summary of the updates

  • JavaSparkSQL.java file was removed. I kept it initially since the file itself was quite old (2+ years) and it was present in your original WIP branch alongside the new file. But I can confirm that the new file covers the same functionality and more. No need to keep the old one, agree with you.
  • Apache header in JavaSqlDataSourceExample.java was added.
  • $-notation instead of df("...") in Scala examples.
  • col("...") instead of df.col("...") in Java examples.
  • Blank lines before {% include_example programmatic_schema ... } were added. However, everything was rendered fine locally even without them.
  • 2 space indentation for chained method calls. My fault, sorry.
  • Actual outputs for all show() calls were added.
  • Tested manually and via ./dev/run-tests.

Open questions

  • Shall I add blank lines before each {% include_example ... } or only before those two examples?
  • I pointed to a wrong location that exceeded the length limit. It is exactly the same functionality but in Java. So, 113 and 117 lines of the JavaSqlDataSourceExample.java file. In my view, it would make sense to keep them as they are now for the better looking documentation.

@SparkQA
Copy link

SparkQA commented Jul 12, 2016

Test build #62192 has finished for PR 14119 at commit 7451fc7.

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

asfgit pushed a commit that referenced this pull request Jul 13, 2016
- Hard-coded Spark SQL sample snippets were moved into source files under examples sub-project.
- Removed the inconsistency between Scala and Java Spark SQL examples
- Scala and Java Spark SQL examples were updated

The work is still in progress. All involved examples were tested manually. An additional round of testing will be done after the code review.

![image](https://cloud.githubusercontent.com/assets/6235869/16710314/51851606-462a-11e6-9fbe-0818daef65e4.png)

Author: aokolnychyi <[email protected]>

Closes #14119 from aokolnychyi/spark_16303.

(cherry picked from commit 772c213)
Signed-off-by: Cheng Lian <[email protected]>
@liancheng
Copy link
Contributor

liancheng commented Jul 13, 2016

LGTM, I've merged this to master and branch-2.0. Thanks for working on this!

I only observed one weird rendering case caused by the missing blank lines before {% include_example %}, but maybe my local Jekyll version is too low. I think it's fine to leave other lines as is. The exceeded lines in the Java example file should be OK.

Could you please remove the WIP tag from the PR title? (I've removed it manually while merging this PR.)

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.

3 participants