Bug #8488
improve performance of WRITE-JSON
100%
Related issues
History
#2 Updated by Constantin Asofiei about 1 month ago
- Status changed from New to WIP
- Assignee set to Constantin Asofiei
- % Done changed from 0 to 90
- Avoid BDTs within internal FWD runtime.
- Replaced iterators with Java 'for', where it applies.
- Read the temp-table fields directly from the
Record.data
as Java types and not BDT. - Directly set the record in the parent buffer instead of using a findUnique
Eric, you may want to look at the changes in persist.orm
.
Ovidiu, an early review is appreciated - I still need to regression test in standalone tests (especially (de)normalized temp-table extent fields in WRITE-JSON).
#3 Updated by Ovidiu Maxiniuc about 1 month ago
Constantin Asofiei wrote:
Sure. I think the code is good. My observations are as follows:Ovidiu, an early review is appreciated - I still need to regression test in standalone tests (especially (de)normalized temp-table extent fields in WRITE-JSON).
- In
P2JQuery.java
, you addedpublic default boolean _getNext()
which is implemented as:getNext().booleanValue()
. I think we should do the other way around: thegetNext()
method to be 'final' (evidently cannot be) andreturn new logical(_getNext())
. The implementing classes should use_getNext()
return Java native value. You did this whole implementation inQueryWrapper
, I see. - In
RecordBuffer.java
. We attempted to make the public API as small as possible. DoesloadRecord()
really need to be public? - Related to above,
Property.java
, the new back-reference toPropertyMeta meta
ispublic
. I know that this is probably not the final form, maybe we can find a solution which better encapsulates and grants the new member a const/final
feeling? JsonExport.java
, line 1466/1467, I think they can be written asdate p2jDate = new datetimetz((OffsetDateTime) d);
, avoiding changing the value ofd
argument of the lambda. IMO, it is ugly to change thed
's type (and its value, of course) that way. The same fordatetime
anddate
, below;- the
XmlExport
might also benefit from the same boost asJsonExport
.
#4 Updated by Constantin Asofiei about 1 month ago
Ovidiu, in rev 15080 I have the review fixes. See bellow for notes:
- In
P2JQuery.java
, you addedpublic default boolean _getNext()
which is implemented as:getNext().booleanValue()
. I think we should do the other way around: thegetNext()
method to be 'final' (evidently cannot be) andreturn new logical(_getNext())
. The implementing classes should use_getNext()
return Java native value. You did this whole implementation inQueryWrapper
, I see.
Done
- In
RecordBuffer.java
. We attempted to make the public API as small as possible. DoesloadRecord()
really need to be public?
Yes, it's used from JsonExport
, which is in a different package.
- Related to above,
Property.java
, the new back-reference toPropertyMeta meta
ispublic
. I know that this is probably not the final form, maybe we can find a solution which better encapsulates and grants the new member a const/final
feeling?
Fixed.
JsonExport.java
, line 1466/1467, I think they can be written asdate p2jDate = new datetimetz((OffsetDateTime) d);
, avoiding changing the value ofd
argument of the lambda. IMO, it is ugly to change thed
's type (and its value, of course) that way. The same fordatetime
anddate
, below;
Fixed.
- the
XmlExport
might also benefit from the same boost asJsonExport
.
I agree, but not in this task.
I'm placing this in ETF testing.
#5 Updated by Constantin Asofiei about 1 month ago
- Status changed from WIP to Review
- % Done changed from 90 to 100
#6 Updated by Greg Shah about 1 month ago
- Related to Feature #8494: rework XmlExport with the performance improvements from JsonExport added
#7 Updated by Constantin Asofiei about 1 month ago
- Status changed from Review to Internal Test
#8 Updated by Greg Shah about 1 month ago
I'm good with that plan.
#9 Updated by Ovidiu Maxiniuc about 1 month ago
I have re-review the 8488a up to r15080 and I am OK with the changes.
The branch can be merged to trunk.
#10 Updated by Constantin Asofiei about 1 month ago
- Status changed from Internal Test to Merge Pending
Merging now.
#11 Updated by Constantin Asofiei about 1 month ago
- Status changed from Merge Pending to Test
Branch 8488a was merged into trunk as rev. 15084 and archived.