Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/8111#issuecomment-130133925
Reviewed 9 of 131 files at r1, 35 of 102 files at r5, 1 of 2 files at r8.
Review status: 78 of 132 files reviewed at latest revision, 12 unresolved
discussions, all commit checks successful.
---
<sup>**[sql/core/src/test/scala/org/apache/spark/sql/sources/DDLTestSuite.scala,
line 74
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwUYi2rNZvbRoYv6iWE)**
([raw
file](https://github.com/apache/spark/blob/e1e513e0af646588cdceaa6abc75487025247aaf/sql/core/src/test/scala/org/apache/spark/sql/sources/DDLTestSuite.scala#L74)):</sup>
How are the before and after methods ordered here? I wonder if we should
re-write this to `override beforeAll()` to make this more explicit, since the
relative ordering of `before { } ` and `beforeAll` isn't immediately apparent.
---
<sup>**[sql/core/src/test/scala/org/apache/spark/sql/sources/FilteredScanSuite.scala,
line 104
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwUYyv5Rn0y2FBExvmG)**
([raw
file](https://github.com/apache/spark/blob/e1e513e0af646588cdceaa6abc75487025247aaf/sql/core/src/test/scala/org/apache/spark/sql/sources/FilteredScanSuite.scala#L104)):</sup>
Same comment applies here RE: relative ordering of the before methods.
---
<sup>**[sql/core/src/test/scala/org/apache/spark/sql/sources/PrunedScanSuite.scala,
line 59
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwUZ5j4GKLaFzPhd146)**
([raw
file](https://github.com/apache/spark/blob/e1e513e0af646588cdceaa6abc75487025247aaf/sql/core/src/test/scala/org/apache/spark/sql/sources/PrunedScanSuite.scala#L59)):</sup>
Another `before` case.
---
<sup>**[sql/core/src/test/scala/org/apache/spark/sql/sources/SaveLoadSuite.scala,
line 48
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwUZC86QQEIM-SGDjGV)**
([raw
file](https://github.com/apache/spark/blob/e1e513e0af646588cdceaa6abc75487025247aaf/sql/core/src/test/scala/org/apache/spark/sql/sources/SaveLoadSuite.scala#L48)):</sup>
As a general rule, do you think that we should wrap this in a `try-finally`
so that the superclass's teardown is guaranteed to be called even if an error
occurs in the subclass's teardown?
---
<sup>**[sql/hive/src/test/java/test/org/apache/spark/sql/hive/JavaDataFrameSuite.java,
line 36
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwUZzLqWATZHKJxddHO)**
([raw
file](https://github.com/apache/spark/blob/e1e513e0af646588cdceaa6abc75487025247aaf/sql/hive/src/test/java/test/org/apache/spark/sql/hive/JavaDataFrameSuite.java#L36)):</sup>
Import ordering?
---
<sup>**[sql/hive/src/test/scala/org/apache/spark/sql/hive/parquetSuites.scala,
line 425
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwU_kkbimhWn7iCKMKm)**
([raw
file](https://github.com/apache/spark/blob/e1e513e0af646588cdceaa6abc75487025247aaf/sql/hive/src/test/scala/org/apache/spark/sql/hive/parquetSuites.scala#L425)):</sup>
Why is this necessary? Is the problem that `ctx` is a `def`?
---
<sup>**[sql/README.md, line 63
\[r8\]](https://reviewable.io:443/reviews/apache/spark/8111#-JwUaSPq07n0nP3rz_vh)**
([raw
file](https://github.com/apache/spark/blob/afa757c98c537965007cad4c61c436887f3ac6a6/sql/README.md#L63)):</sup>
I suppose that we might want to update this to reflect the new output
printed by `build/sbt hive/console`; might as well do so if we're going to have
to manually test this anyways.
---
Comments from the [review on
Reviewable.io](https://reviewable.io:443/reviews/apache/spark/8111)
<!-- Sent from Reviewable.io -->
---
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 is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
