I9996 value limits it fails oracle #10027
Conversation
CHANGELOG_BEGIN CHANGELOG_END
CHANGELOG_BEGIN CHANGELOG_END
danielporterda
requested review from
gerolf-da,
mziolekda,
nmarton-da,
rautenrieth-da and
tudor-da
as code owners
CHANGELOG_BEGIN CHANGELOG_END
CHANGELOG_BEGIN CHANGELOG_END
| override def arrayIntersectionWhereClause(arrayColumn: String, parties: Set[Party]): String = | ||
| s"""JSON_EXISTS($arrayColumn, '$$[*]?(@ in ("${parties.mkString("""", """")}"))')""" | ||
| override def arrayIntersectionWhereClause(arrayColumn: String, parties: Set[Party]): String = { | ||
| val NumCharsBetweenParties = 3 |
There was a problem hiding this comment.
Is it worth actually parsing to get character lengths from the live expression on line 86 so these checks don't rot if the expression is changed in future?
| val OracleMaxStringLiteralLength = 4000 | ||
|
|
||
| val groupedParties = parties.toList.sorted.foldLeft((List.empty[List[String]], 0))({ | ||
| case ((prev, currentLength), party) => |
There was a problem hiding this comment.
why prev rather than next?
| ) { | ||
| (List(party) :: prev, party.length + NumExtraChars) | ||
| } else { | ||
| prev match { |
There was a problem hiding this comment.
worth adding a unit test for this?
CHANGELOG_BEGIN CHANGELOG_END
| @@ -62,8 +62,31 @@ private[dao] object SqlFunctions { | |||
| object OracleSqlFunctions extends SqlFunctions { | |||
| // TODO https://github.com/digital-asset/daml/issues/9493 | |||
| // This is likely extremely inefficient due to the multiple full tablescans on unindexed varray column | |||
| override def arrayIntersectionWhereClause(arrayColumn: String, parties: Set[Party]): String = | |||
| s"""JSON_EXISTS($arrayColumn, '$$[*]?(@ in ("${parties.mkString("""", """")}"))')""" | |||
| override def arrayIntersectionWhereClause(arrayColumn: String, parties: Set[Party]): String = { | |||
There was a problem hiding this comment.
TLDR: do you really need this now? this might not needed later
the root problem here might be something else: this method for constructing the query is totally static, meaning: it is not like we have a prepare statement with something like ? in the middle which we pass to the prepared statement as a data -what we should be doing-, but rather we have a prepared statement where we statically string interpolate all the data (look for the # where this is used).
I can very well imagine if we would be able to pass the all parties as an array, we wouldn't have this problem.
Which would mean if we change this approach accordingly (this is on our horizon), this change will be lost.
So question is how bad you need this working on a short run? (if you do I am completely happy to go with it temporarily, although the complexity here scares me a bit)
There was a problem hiding this comment.
@nmarton-da without this fix the ValueLimitsIt.scala fails on oracle for sandbox-on-x (that is what triggered the discovery) so this is defensiveness to deal with a big party list of submitters without requiring a hard limit.
@danielporterda also contemplated moving to a ? prepared statement parameter replacement approach but that would have had a bigger knock-on impact to interfaces and across the stack which we deemed too impactful given all the moving parts happening now.
I would vote for merging this on the existing Oracle schema and then when we can revisit as we implement the equiv on the new append only.
There was a problem hiding this comment.
yeah, I'm certainly not in love with this course of action, and I share your assessment of what the "root problem" is, but it felt like best of bad options for the moment. As Brian says, it's motivated by making the test pass. The other two possible implementations are
- use a prepared statement as you suggest, or
- render the JSON fields as a materialized view and do a subquery
It's not totally clear if a prepared statement will get around the issues with the string length, but that's worth investigating. The reason it felt like a non-starter was having to change a bunch of interfaces while the append only work is in motion.
As far as rendering the JSON array fields as a materialized view, this is appealing because it (should) mean that we can create an index on them, however @stephencompall-DA apparently ran into some issues with this that may or may not have implications about correctness - some issue with filtering out duplicates. This is second hand, but the upshot was that it was a larger change that wasn't guaranteed to work, and wasn't clear how it was going to play with your changes.
In general, I'm largely opposed to anything that depends on getting arithmetic correct, given how much evidence there is that programmers are terrible at it. Hard to get right, harder still to modify later. +1 to improving this later.
There was a problem hiding this comment.
all what you said makes sense to me, sorry guys for not being much constructive here, let's improve on this later 👌
There was a problem hiding this comment.
No worries, if you see me doing what looks like a footgun in the future, please feel free to speak up. Would prefer to explain myself over accidentally doing bad stuff. I'm not always so verbose, scout's honor :)
* Creating OR predicate to reduce length of string literals CHANGELOG_BEGIN CHANGELOG_END * removing unnecessary thrown exception CHANGELOG_BEGIN CHANGELOG_END * switching to camel case for constants CHANGELOG_BEGIN CHANGELOG_END * run format CHANGELOG_BEGIN CHANGELOG_END * remove tolist conversion CHANGELOG_BEGIN CHANGELOG_END * scalafmt Co-authored-by: Brian Healey <brian.healey@digitalasset.com>
This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @S11001001 is in charge of this release. Commit log: ``` d867d90 define query offset semantics solely in terms of #9847 per-query offsets (#10071) 1f5aa44 [JSON-API] Add metrics for ledger command submission timing (#10076) 7eb2ce7 update compat versions for 1.15.0-snapshot.20210615.7169.0.adeba206 (#10028) c44f337 Simplify speedy benchmark (#10079) 58b1c4e Speedy: Refactor contract Id/Key callback (#10033) 8558c73 Upgrade canton to 0.25.0 based on daml 1.14.0 (#10074) e4c1c58 Run scenarios in off-ledger machine (#10070) 31a76a4 allow CI pools to use any zone (#10069) 0391f35 [JSON-API] Add db metrics & response construction metrics (#10068) ea089ad clean up local VSCode after release (#10065) 39a5890 fix 9_functional redirect (#10067) fb757d8 Handle visibility outside of speedy (#10056) b796695 Add per-query offsets to HTTP-JSON API (#9847) ab8bac5 [In-memory fan-out] BuffersUpdater subscribes from the ledger end (#10037) e458529 [JSON-API] Add moar timing metrics (#10045) e12a449 s/DABL/Daml Hub/ (#10062) a205d0e Enriched conformance tests for transactions (trees) streams (#9977) b98ad91 Bump test sha (#10055) f3106c3 I9996 value limits it fails oracle (#10027) febca5d Use ScenarioRunner.submit in Daml Script (#10053) 3113702 index json-api signatories and observers on Oracle (#9895) cb3a42a update NOTICES file (#10047) 2f51869 Allow constraints in any position in data-deps. (#10049) 7dfa36f pass supportedJdbcDriverNames implicitly, and only to what actually needs it (#10036) 0fbc1ee H2 support for append only schema (part2) [DPP-394] (#10030) cd2fda6 Fix broken package.json config (#10044) 10ab05f Fix vsce publishing (#10043) 090dd95 rotate release duty after 1.14.0-snapshot.20210615.7169.0.adeba206 (#10019) eee484b Implemented in-memory buffers prunning in BaseLedger (#9936) b96639e Do not terminate streams when no end-offset given in `ledger-api-bench-tool` [DPP-422] (#10035) 2b915e9 Split scenario & ledger execution (#10039) 591176c Remove SqlSequence, cleanup code (#10042) 8ea240f Release SDK 1.14.0 (#10041) 3532460 LF: Structure Preprocessing Errors (#10013) 1852830 [JSON-API] Concurrent query etc. metrics (#10031) 321c4da test different template-ID frequencies in the "mega" perf scenario (#10016) 993591e truncate party name for 64 chars for multi-valued submitters to avoid… (#10000) 7a2a349 Log context of all updates written to the database (#10010) 4005c84 release 1.15.0-snapshot.20210615.7169.0.adeba206 (#10022) d1aa256 Force css-what resolution in compiler/daml-extension & fully regenera… (#10025) 0fbfd99 DPP-394 h2 support for append only schema (#10009) 32d70cf Revert "release 1.14.0-snapshot.20210615.7169.0.adeba206 (#10018)" (#10023) cfee5a2 [JSON-API] Add information from the jwtpayload to the logging context (#9995) fe63825 release 1.14.0-snapshot.20210615.7169.0.adeba206 (#10018) ``` Changelog: ``` - [JSON-API] Timing metrics which measure how long the processing of a command submission request takes on the ledger are now available - [JSON-API] The database operations (regardless of in-memory or postgres) contain now metrics for fetching contracts by id or key (seperate metrics foreach) - [JSON-API] The time for a repsonse payload construction of a request is now also tracked in a metric [HTTP/JSON API] The streaming query endpoint now accepts to override the offset for each query using the `offset` field. You can read more about it on the documentation. - [JSON-API] Timing metrics are now available for party management, package management, command submission and query endpoints. - [JSON-API] Also added a timing metric for parsing and decoding of incoming json payloads - [JSON API] The Oracle database schema has changed; if using ``--query-store-jdbc-config``, you must rebuild the database by adding ``,createSchema=true``. See #9895. - [JSON-API] The metrics which describe the amount of these concurrent events is now available: - running http request - command submissions - package allocations - party allocations [metrics] Limit size of multi party metrics to ease consumption For every update in the index db log the full context at the INFO level. - [JSON API] For applicable requests actAs, readAs, applicationId & ledgerId are included in the log context ``` CHANGELOG_BEGIN CHANGELOG_END
Pull Request Checklist
CHANGELOG_BEGINandCHANGELOG_ENDtagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.