deephaven / deephaven-core

Deephaven Community Core
Other
256 stars 80 forks source link

Unable to reference byte array column in custom column formula #6096

Open devinrsmith opened 2 months ago

devinrsmith commented 2 months ago

I'm unable to use a byte[] column type in custom column formulas. Here's the setup:

from deephaven import new_table

x = (
    new_table({"foo": ["Bar"]})
    .view(["foo_bytes=foo.getBytes()"])
)

Now, create a custom column in the web UI column name foo_str and the formula new java.lang.String(foo_bytes). The relevant error (only visible in server logs):

Cannot find constructor for java.lang.String(io.deephaven.engine.table.impl.preview.ArrayPreview)

It looks like the web client is trying to build off of the web UIs derivation of the table (ie, the ArrayPreview column type) as opposed to the original table (ie, the byte[] column type).

image

Here's the full server side exception:

2024-09-20T15:10:05.932Z | heduler-Concurrent-3 | ERROR | i.d.s.s.SessionService    | Internal Error '05ebc0af-c75d-4e69-9099-d51c874102d6' io.deephaven.engine.table.impl.select.FormulaCompilationException: Formula compilation error for: new java.lang.String(foo_bytes)
        at io.deephaven.engine.table.impl.select.DhFormulaColumn.initDef(DhFormulaColumn.java:201)
        at io.deephaven.engine.table.impl.select.SwitchColumn.initDef(SwitchColumn.java:64)
        at io.deephaven.engine.table.impl.select.analyzers.SelectAndViewAnalyzer.createContext(SelectAndViewAnalyzer.java:128)
        at io.deephaven.engine.table.impl.QueryTable.validateSelect(QueryTable.java:1500)
        at io.deephaven.server.table.validation.ColumnExpressionValidator.validateColumnExpressions(ColumnExpressionValidator.java:105)
        at io.deephaven.server.table.ops.UpdateOrSelectGrpcImpl.create(UpdateOrSelectGrpcImpl.java:52)
        at io.deephaven.server.table.ops.UpdateOrSelectGrpcImpl$UpdateViewGrpcImpl.create(UpdateOrSelectGrpcImpl.java:90)
        at io.deephaven.server.table.ops.TableServiceGrpcImpl$BatchExportBuilder.doExport(TableServiceGrpcImpl.java:757)
        at io.deephaven.server.session.SessionState$ExportObject.doExport(SessionState.java:995)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
        at io.deephaven.server.runner.scheduler.SchedulerModule$ThreadFactory.lambda$newThread$0(SchedulerModule.java:100)
        at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: io.deephaven.engine.table.impl.lang.QueryLanguageParser$QueryLanguageParseException: 

Having trouble with the following expression:
Full expression           : new java.lang.String(foo_bytes)
Expression having trouble : new java.lang.String
Exception type            : io.deephaven.engine.table.impl.lang.QueryLanguageParser$ParserResolutionFailure
Exception message         : Cannot find constructor for java.lang.String(io.deephaven.engine.table.impl.preview.ArrayPreview) in class java.lang.String

        at io.deephaven.engine.table.impl.lang.QueryLanguageParser.getConstructor(QueryLanguageParser.java:859)
        at io.deephaven.engine.table.impl.lang.QueryLanguageParser.visit(QueryLanguageParser.java:2851)
        at io.deephaven.engine.table.impl.lang.QueryLanguageParser.visit(QueryLanguageParser.java:133)
        at com.github.javaparser.ast.expr.ObjectCreationExpr.accept(ObjectCreationExpr.java:112)
        at io.deephaven.engine.table.impl.lang.QueryLanguageParser.<init>(QueryLanguageParser.java:319)
        at io.deephaven.engine.table.impl.lang.QueryLanguageParser.<init>(QueryLanguageParser.java:212)
        at io.deephaven.engine.table.impl.select.codegen.FormulaAnalyzer.parseFormula(FormulaAnalyzer.java:240)
        at io.deephaven.engine.table.impl.select.codegen.FormulaAnalyzer.parseFormula(FormulaAnalyzer.java:122)
        at io.deephaven.engine.table.impl.select.DhFormulaColumn.initDef(DhFormulaColumn.java:181)
        ... 15 more
mofojed commented 2 months ago

When applying the request, I'm pretty sure the table it's applying to has the preview columns already: https://github.com/deephaven/deephaven-core/blob/77cbe4c44ecf030fce946df333a60f246d4c8057/server/src/main/java/io/deephaven/server/table/ops/UpdateOrSelectGrpcImpl.java#L49 Source table can be found via the PREVIEW_PARENT_TABLE attribute: https://github.com/deephaven/deephaven-core/blob/77cbe4c44ecf030fce946df333a60f246d4c8057/engine/table/src/main/java/io/deephaven/engine/table/impl/preview/ColumnPreviewManager.java#L115 So I think it needs to check if there's a source table, apply the custom columns to that, and then apply previews again before returning the table?

Doesn't seem to be a UI issue though, as we don't know the custom column contains another column in it's express at all, we're just sending the spec as a string to the server.

rcaudy commented 2 months ago

So, the thought is to change the server side implementation to detect that there's a parent to apply to?

devinrsmith commented 2 months ago

It's the web UI that is calling

  /*
   * Create a table that has preview columns applied to an existing source table.
   */
  rpc ApplyPreviewColumns(ApplyPreviewColumnsRequest) returns (ExportedTableCreationResponse) {}

though?

In which case, I might argue it should still be web UI to keep reference to parent? It's technically possible that a caller in another context would actually want to refer to an ArrayPreview type, and so unconditionally grabbing the parent on the server side might be incorrect.

devinrsmith commented 2 months ago

Or maybe, makes it the responsibility of the JS layer; still, not sure if we'd want to unconditionally unwrap parent of said table.

niloc132 commented 2 months ago

This is a longstanding bug, inherited (where it was by design) from DHE, and up until now DHC has applied it consistently with DHE.

It isnt the web UI calling that, but the JS API, under the assumption that it needs to be done so that we don't end up with tables where the client either can't read the contents, or the ui can't display them.

With #5890 merged, we can change this, since we can support more types on the client. See also https://github.com/deephaven/deephaven-core/issues/5708, https://github.com/deephaven/deephaven-core/issues/2102.

The fix will probably be to apply it conditionally at the time of the subscription being created. Making it optional will mean exposing more flags when creating subscriptions from the UI, both to let it be enabled/disabled, but also (2102 above) supporting limits on arrays to be sent, etc.