[SPARK-23462][SQL] improve missing field error message in `StructType` by xysun · Pull Request #20649 · apache/spark

xysun · 2018-02-21T09:34:21Z

What changes were proposed in this pull request?

The error message s"""Field "$name" does not exist.""" is thrown when looking up an unknown field in StructType. In the error message, we should also contain the information about which columns/fields exist in this struct.

How was this patch tested?

Added new unit tests.

Note: I created a new StructTypeSuite.scala as I couldn't find an existing suite that's suitable to place these tests. I may be missing something so feel free to propose new locations.

Please review http://spark.apache.org/contributing.html before opening a pull request.

xysun · 2018-02-21T09:34:50Z

@gatorsmile please review. Thanks!

gatorsmile · 2018-02-21T22:18:54Z

ok to test

SparkQA · 2018-02-22T01:47:36Z

Test build #87599 has finished for PR 20649 at commit e1a5ccf.

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

gatorsmile · 2018-02-22T02:34:23Z

+import org.apache.spark.SparkFunSuite
+import org.apache.spark.sql.types.StructType
+
+class StructTypeSuite extends SparkFunSuite{


Add a space before {

gatorsmile · 2018-02-22T02:34:38Z

+class StructTypeSuite extends SparkFunSuite{
+
+ test("SPARK-23462 lookup a single missing field should output existing fields") {
+ val s = StructType.fromDDL("a INT")


add one more column?

SparkQA · 2018-02-22T07:26:08Z

Test build #87608 has finished for PR 20649 at commit 22f89f8.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class StructTypeSuite extends SparkFunSuite

HyukjinKwon · 2018-02-22T08:28:01Z

+import org.apache.spark.SparkFunSuite
+import org.apache.spark.sql.types.StructType
+
+class StructTypeSuite extends SparkFunSuite {


Could we put this file under sql/catalyst/src/test/scala/org/apache/spark/sql/types or move the test cases into DataTypeSuite?

mgaido91 · 2018-02-23T13:53:56Z

- throw new IllegalArgumentException(s"""Field "$name" does not exist."""))
+ throw new IllegalArgumentException(
+ s"""Field "$name" does not exist.
+ |Available fields: ${fieldNamesSet.mkString(",")}""".stripMargin))


nit: can we add a space after the comma? I think it can help readability

mgaido91 · 2018-02-23T13:54:02Z

 throw new IllegalArgumentException(
- s"Field ${nonExistFields.mkString(",")} does not exist.")
+ s"""Fields ${nonExistFields.mkString(",")} does not exist.
+ |Available fields: ${fieldNamesSet.mkString(",")}""".stripMargin)


mgaido91 · 2018-02-23T13:54:17Z

 if (nonExistFields.nonEmpty) {
 throw new IllegalArgumentException(
- s"Field ${nonExistFields.mkString(",")} does not exist.")
+ s"""Fields ${nonExistFields.mkString(",")} does not exist.


mgaido91 · 2018-02-23T13:54:29Z

- throw new IllegalArgumentException(s"""Field "$name" does not exist."""))
+ throw new IllegalArgumentException(
+ s"""Field "$name" does not exist.
+ |Available fields: ${fieldNamesSet.mkString(",")}""".stripMargin))


mgaido91 · 2018-02-23T13:55:26Z

+ val e = intercept[IllegalArgumentException](s.fieldIndex("c")).getMessage
+ assert(e.contains("Available fields: a,b"))
+ }
+


nit: this empty line should be removed

kiszk · 2018-02-23T14:32:29Z

+
+class StructTypeSuite extends SparkFunSuite {
+
+ test("SPARK-23462 lookup a single missing field should output existing fields") {


nit: Would it be better to combine these three tests into one? It could reduce the total test time.

This is not a bug fix, so please don't include the JIRA number in the test case description.

removed JIRA number.
@kiszk i still prefer to keep separate tests as they are calling separate methods. Let me know if you feel strongly against it.

SparkQA · 2018-02-28T08:05:02Z

Test build #87759 has finished for PR 20649 at commit 8cdb1d5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class StructTypeSuite extends SparkFunSuite

xysun · 2018-02-28T08:59:40Z

~~I'm getting some weird compile errors on local even when based off previous successful commit.. checking..~~

xysun · 2018-02-28T09:19:05Z

retest this please

gatorsmile · 2018-02-28T17:29:58Z

retest this please

SparkQA · 2018-02-28T20:58:51Z

Test build #87792 has finished for PR 20649 at commit 8cdb1d5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class StructTypeSuite extends SparkFunSuite

xysun · 2018-03-04T14:38:20Z

hey @gatorsmile @kiszk @jiangxb1987 @HyukjinKwon @mgaido91 any more comments?

jiangxb1987 · 2018-03-05T02:43:30Z

 if (nonExistFields.nonEmpty) {
 throw new IllegalArgumentException(
- s"Field ${nonExistFields.mkString(",")} does not exist.")
+ s"""Fields ${nonExistFields.mkString(", ")} does not exist.


nit: nonExistFields can contain only one field, so maybe Fields -> Field(s)?

"Field(s) ... do(es) not exist." like this? A bit weird...
maybe just "Fields ... do not exist"?

Not existing field(s): ${nonExistFields.mkString(", ")} ?

jiangxb1987 · 2018-03-05T02:50:35Z

LGTM

SparkQA · 2018-03-05T08:05:02Z

Test build #87952 has finished for PR 20649 at commit b2d1888.

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

xysun · 2018-03-05T08:13:43Z

:( can anyone help issue the retest command? @mgaido91 @gatorsmile thanks!

HyukjinKwon · 2018-03-05T08:16:05Z

retest this please

SparkQA · 2018-03-05T11:37:58Z

Test build #87953 has finished for PR 20649 at commit b2d1888.

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

xysun · 2018-03-06T01:48:16Z

hey @HyukjinKwon @gatorsmile @mgaido91 anything else you'd like to comment/change? else are we good to merge?
Thanks!

HyukjinKwon · 2018-03-06T03:53:59Z

I usually leave open it for few more days in case other reviewers have some more review comments.

jiangxb1987 · 2018-03-08T05:35:16Z

LGTM, cc @gatorsmile @cloud-fan

gatorsmile · 2018-03-08T05:56:46Z

retest this please

cloud-fan · 2018-03-08T06:07:31Z

- throw new IllegalArgumentException(s"""Field "$name" does not exist."""))
+ throw new IllegalArgumentException(
+ s"""Field "$name" does not exist.
+ |Available fields: ${fieldNamesSet.mkString(", ")}""".stripMargin))


shall we use fieldNames to keep the field order?

SparkQA · 2018-03-08T08:05:01Z

Test build #88076 has finished for PR 20649 at commit b2d1888.

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

HyukjinKwon · 2018-03-10T10:50:20Z

ping @xysun

xysun · 2018-03-12T02:16:22Z

Latest code review fixes pushed. cc @HyukjinKwon

SparkQA · 2018-03-12T05:39:38Z

Test build #88166 has finished for PR 20649 at commit 78e037d.

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

HyukjinKwon · 2018-03-12T13:13:58Z

Merged to master and branch-2.3.

## What changes were proposed in this pull request? The error message ```s"""Field "$name" does not exist."""``` is thrown when looking up an unknown field in StructType. In the error message, we should also contain the information about which columns/fields exist in this struct. ## How was this patch tested? Added new unit tests. Note: I created a new `StructTypeSuite.scala` as I couldn't find an existing suite that's suitable to place these tests. I may be missing something so feel free to propose new locations. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Xiayun Sun <xiayunsun@gmail.com> Closes#20649 from xysun/SPARK-23462. (cherry picked from commit b304e07) Signed-off-by: hyukjinkwon <gurwls223@gmail.com>

## What changes were proposed in this pull request? The error message ```s"""Field "$name" does not exist."""``` is thrown when looking up an unknown field in StructType. In the error message, we should also contain the information about which columns/fields exist in this struct. ## How was this patch tested? Added new unit tests. Note: I created a new `StructTypeSuite.scala` as I couldn't find an existing suite that's suitable to place these tests. I may be missing something so feel free to propose new locations. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Xiayun Sun <xiayunsun@gmail.com> Closesapache#20649 from xysun/SPARK-23462.

## What changes were proposed in this pull request? The error message ```s"""Field "$name" does not exist."""``` is thrown when looking up an unknown field in StructType. In the error message, we should also contain the information about which columns/fields exist in this struct. ## How was this patch tested? Added new unit tests. Note: I created a new `StructTypeSuite.scala` as I couldn't find an existing suite that's suitable to place these tests. I may be missing something so feel free to propose new locations. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Xiayun Sun <xiayunsun@gmail.com> Closesapache#20649 from xysun/SPARK-23462. (cherry picked from commit b304e07) Signed-off-by: hyukjinkwon <gurwls223@gmail.com>

output current fields in missing fields error message
e1a5ccf

xysun changed the title ~~[SPARK-23452][SQL] improve missing field error message in StructType~~[SPARK-23462][SQL] improve missing field error message in StructTypeFeb 21, 2018

gatorsmile reviewed Feb 22, 2018
View reviewed changes

formatting; test with multi-column StructType
22f89f8

HyukjinKwon reviewed Feb 22, 2018
View reviewed changes

mgaido91 reviewed Feb 23, 2018
View reviewed changes

kiszk reviewed Feb 23, 2018
View reviewed changes

xysun added 2 commits February 28, 2018 13:59

code review fix
0842eeb

move
8cdb1d5

HyukjinKwon approved these changes Mar 4, 2018
View reviewed changes

jiangxb1987 reviewed Mar 5, 2018
View reviewed changes

grammar
b2d1888

cloud-fan reviewed Mar 8, 2018
View reviewed changes

use fieldNames to preserve order
78e037d

asfgit closed this in b304e07Mar 12, 2018

GulajavaMinistudio mentioned this pull request Mar 12, 2018
Update upstream GulajavaMinistudio/spark#312
Merged


		class StructTypeSuite extends SparkFunSuite {

		test("SPARK-23462 lookup a single missing field should output existing fields") {

Uh oh!

Conversation

xysun commented Feb 21, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

xysun commented Feb 21, 2018

Uh oh!

gatorsmile commented Feb 21, 2018

Uh oh!

SparkQA commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

xysun commented Feb 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xysun commented Feb 28, 2018

Uh oh!

gatorsmile commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

xysun commented Mar 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xysunMar 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Mar 5, 2018

Uh oh!

SparkQA commented Mar 5, 2018

Uh oh!

xysun commented Mar 5, 2018

Uh oh!

HyukjinKwon commented Mar 5, 2018

Uh oh!

SparkQA commented Mar 5, 2018

Uh oh!

xysun commented Mar 6, 2018

Uh oh!

xysun commented Feb 28, 2018•
edited
Loading

xysun commented Mar 4, 2018•
edited
Loading

xysunMar 5, 2018•
edited
Loading