Skip to content

Conversation

@ueshin
Copy link
Member

@ueshin ueshin commented Mar 8, 2023

What changes were proposed in this pull request?

Fixes spark.sql to return values from the command.

Why are the changes needed?

Currently spark.sql doesn't return the result from the commands.

>>> spark.sql("show functions").show()
+--------+
|function|
+--------+
+--------+

Does this PR introduce any user-facing change?

spark.sql with commands will return the values.

How was this patch tested?

Added a test.

@LuciferYang
Copy link
Contributor

Is there a similar case on Scala connect client ?

@ueshin
Copy link
Member Author

ueshin commented Mar 8, 2023

Is there a similar case on Scala connect client ?

I haven't tried Scala client, but yes, it would happen, and this will fix both.

@LuciferYang
Copy link
Contributor

Is there a chance to add a similar case in ClientE2ETestSuite?

Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@zhengruifeng
Copy link
Contributor

@LuciferYang This PR fix it in the connect planner, so should also works for the Scala Client.

@LuciferYang
Copy link
Contributor

@LuciferYang This PR fix it in the connect planner, so should also works for the Scala Client.

OK, got it

zhengruifeng pushed a commit that referenced this pull request Mar 8, 2023
### What changes were proposed in this pull request?

Fixes `spark.sql` to return values from the command.

### Why are the changes needed?

Currently `spark.sql` doesn't return the result from the commands.

```py
>>> spark.sql("show functions").show()
+--------+
|function|
+--------+
+--------+
```

### Does this PR introduce _any_ user-facing change?

`spark.sql` with commands will return the values.

### How was this patch tested?

Added a test.

Closes #40323 from ueshin/issues/SPARK-42705/sql.

Authored-by: Takuya UESHIN <[email protected]>
Signed-off-by: Ruifeng Zheng <[email protected]>
(cherry picked from commit 1507a52)
Signed-off-by: Ruifeng Zheng <[email protected]>
@zhengruifeng
Copy link
Contributor

thank you all, merged into master/branch-3.4

timeZoneId)
assert(batches.size == 1)
batches.next()
assert(batches.hasNext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late reply but how is this code different to the existing one?

val bytes = batches.next()
bytes

Is the same as

batches.next()

The asserts in between don't count as they don't have side effects.

Copy link
Member Author

Choose a reason for hiding this comment

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

The batches is an iterator, so batches.size consumes all the data in the iterator to calculate the size.
Then batches.next() would return an empty data? Usually it should throw an Exception, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see thank you. My understanding with iterators was that non rewindable iterators simply do not have a size method.

But something new learned.

snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
### What changes were proposed in this pull request?

Fixes `spark.sql` to return values from the command.

### Why are the changes needed?

Currently `spark.sql` doesn't return the result from the commands.

```py
>>> spark.sql("show functions").show()
+--------+
|function|
+--------+
+--------+
```

### Does this PR introduce _any_ user-facing change?

`spark.sql` with commands will return the values.

### How was this patch tested?

Added a test.

Closes apache#40323 from ueshin/issues/SPARK-42705/sql.

Authored-by: Takuya UESHIN <[email protected]>
Signed-off-by: Ruifeng Zheng <[email protected]>
(cherry picked from commit 1507a52)
Signed-off-by: Ruifeng Zheng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants