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.

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

Partition-selection drop-downs don't work properly for non-String columns #1441

Closed
rcaudy opened this issue Aug 8, 2023 · 3 comments · Fixed by #1533 or deephaven/deephaven-core#4559
Assignees
Labels
bug Something isn't working

Comments

@rcaudy
Copy link
Member

rcaudy commented Aug 8, 2023

Description

When using non-String partitioning columns (long in my example), the partition-selection drop-down incorrectly tries to filter as if the values are strings. It wraps them in quotes, causing MatchFilter parsing to reject the literal.

Steps to reproduce

Use a command like:

t=io.deephaven.parquet.table.ParquetTools.readTable(path)

to load a Parquet table that uses metadata files, a TableDefinition, or inferred partitioning column types from key=value directory name pairs. If any of the partitioning column types is not a String, you should see this error or similar.

Note that the automatic first-partition retrieval fails, as does selecting a partition to filter to. coalesce() or where() on the table to skip this servers as a workaround.

Expected results

The table to render properly.

Actual results

A table widget with an error loaded. See screeenshot.

Additional details and attachments

Here's the exception stack trace from the server-side, as displayed in the console:

heduler-Concurrent-3 | i.d.s.s.SessionService    | Internal Error 'ef498c1c-f351-4b5a-9f2b-da5f85b0e713' java.lang.IllegalArgumentException: Failed to convert literal value <"100"> for column "cnum" of type long
	at io.deephaven.engine.table.impl.select.MatchFilter.init(MatchFilter.java:155)
	at io.deephaven.engine.table.impl.PartitionAwareSourceTable.whereImpl(PartitionAwareSourceTable.java:288)
	at io.deephaven.engine.table.impl.PartitionAwareSourceTable.where(PartitionAwareSourceTable.java:272)
	at io.deephaven.engine.table.impl.PartitionAwareSourceTable.where(PartitionAwareSourceTable.java:35)
	at io.deephaven.server.table.ops.FilterTableGrpcImpl.create(FilterTableGrpcImpl.java:57)
	at io.deephaven.server.table.ops.FilterTableGrpcImpl.create(FilterTableGrpcImpl.java:30)
	at io.deephaven.server.table.ops.TableServiceGrpcImpl$BatchExportBuilder.doExport(TableServiceGrpcImpl.java:689)
	at io.deephaven.server.table.ops.TableServiceGrpcImpl.lambda$batch$5(TableServiceGrpcImpl.java:541)
	at io.deephaven.server.session.SessionState$ExportObject.doExport(SessionState.java:921)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at io.deephaven.server.runner.scheduler.SchedulerModule$ThreadFactory.lambda$newThread$0(SchedulerModule.java:78)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.NumberFormatException: For input string: ""100""
	at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
	at java.base/java.lang.Long.parseLong(Long.java:678)
	at java.base/java.lang.Long.parseLong(Long.java:817)
	at io.deephaven.engine.table.impl.select.MatchFilter$ColumnTypeConvertorFactory$4.convertStringLiteral(MatchFilter.java:229)
	at io.deephaven.engine.table.impl.select.MatchFilter.init(MatchFilter.java:152)
	... 15 more

Here's a screenshot of the issue:
Screenshot 2023-08-08 at 9 43 32 AM

Versions

Version
0.27.0

@rcaudy rcaudy added bug Something isn't working triage Issue requires triage labels Aug 8, 2023
@vbabich
Copy link
Collaborator

vbabich commented Aug 9, 2023

@rcaudy Do you have a sample file to make it easier to reproduce?

@nbauernfeind
Copy link
Member

@vbabich here is a reproducer I wrote:

import io.deephaven.engine.util.TableTools
import io.deephaven.parquet.table.ParquetTools

// write a partition table with an int partition column
def part = TableTools.emptyTable(4).update("II = ii")
ParquetTools.writeTable(part, "/tmp/pt-test/intCol=0/part.parquet")
ParquetTools.writeTable(part, "/tmp/pt-test/intCol=1/part.parquet")

// load the partition table
partition_table = ParquetTools.readTable("/tmp/pt-test")

// dump the metadata to the console to see that it is indeed an `intCol`
TableTools.show(partition_table.meta())

@mofojed
Copy link
Member

mofojed commented Sep 18, 2023

Same snippet in Python instead:

from deephaven import empty_table

part = empty_table(4).update("II=ii")

from deephaven.parquet import write, read

write(part, "/tmp/pt-test/intCol=0/part.parquet")
write(part, "/tmp/pt-test/intCol=1/part.parquet")

partition_table = read("/tmp/pt-test")

georgecwan added a commit that referenced this issue Sep 27, 2023
* Partitioned tables were unable to be rendered if the partitioned
column type is not `java.lang.String`
* Grid can now render string, numerical, and character columns
* Fixes partitioning columns filter dropdown to correctly display `char`
types instead of their ASCII values
* The partition search bar function is now able to search for numbers
* Removes the handleFocus function for `PartitionSelectorSearch` since
it made it impossible for the user to edit the search bar after
deselecting it
* Fixes #1441

---------

Co-authored-by: georgecwan <georgecwan@users.noreply.github.com>
mofojed pushed a commit to deephaven/deephaven-core that referenced this issue Sep 27, 2023
Release notes https://github.com/deephaven/web-client-ui/releases/tag/v0.49.1

### Bug Fixes

* Copy did not work from embedded iframes ([#1528](deephaven/web-client-ui#1528)) ([3549a33](deephaven/web-client-ui@3549a33)), closes [#1527](deephaven/web-client-ui#1527)
* Dehydration of class components ([#1535](deephaven/web-client-ui#1535)) ([3e834de](deephaven/web-client-ui@3e834de)), closes [#1534](deephaven/web-client-ui#1534)
* inconsistent drag for webkit ([#1518](deephaven/web-client-ui#1518)) ([cd5408c](deephaven/web-client-ui@cd5408c)), closes [#1360](deephaven/web-client-ui#1360)
* Render tables partitioned by non-string columns ([#1533](deephaven/web-client-ui#1533)) ([585b2ff](deephaven/web-client-ui@585b2ff)), closes [#1441](deephaven/web-client-ui#1441)
* Right clicking with a custom context menu open should open another context menu ([#1526](deephaven/web-client-ui#1526)) ([bd08e1f](deephaven/web-client-ui@bd08e1f)), closes [#1525](deephaven/web-client-ui#1525)

# [0.49.0](deephaven/web-client-ui@v0.48.0...v0.49.0) (2023-09-15)


### Bug Fixes

* Plugin peer dependencies do not get versions from lerna ([#1517](deephaven/web-client-ui#1517)) ([322f6ff](deephaven/web-client-ui@322f6ff))
* Table overflow button has lower priority than grid tokens ([#1510](deephaven/web-client-ui#1510)) ([32e6d20](deephaven/web-client-ui@32e6d20)), closes [#1480](deephaven/web-client-ui#1480)


### Code Refactoring

* Improve table saver to always use the correct service worker ([#1515](deephaven/web-client-ui#1515)) ([2488e52](deephaven/web-client-ui@2488e52)), closes [#766](deephaven/web-client-ui#766)


### Features

* Update go to row panel's row number with cursorRow ([#1508](deephaven/web-client-ui#1508)) ([23ab5cc](deephaven/web-client-ui@23ab5cc)), closes [#1406](deephaven/web-client-ui#1406)


### BREAKING CHANGES

* `TableSaver` now expects the service worker to send it
a complete URL for download instead of just a file name. DHE will need
to adjust its `serviceWorker.js` to incorporate the same changes from
this PR.


Co-authored-by: deephaven-internal <deephaven-internal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants