Change ElementProps acquisition to a prepared statement query

[6ar8nas]

This changes how we fetch ElementProps from the nativeDB. It allows bypassing the creation of a DgnElement on the native side and various json mapping adapters, which should result in both more accurate DB data representation, as well as considerably better Elements API performance.

To support this, a mapping function has been drafted that would allow all consuming applications to use the following dollar syntax query without the need to match the JSON representation to the accurate ElementProps extending interface.

[pmconne]

iModel-native PR: iTwin/imodel-native#688

Instructions for correctly linking your PRs. Don't include "PR", and I'm not sure if it's case-sensitive.

[6ar8nas]

/azp run iTwin.js

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kabentley]

Can you please explain the motivation behind this change?

[6ar8nas]

Can you please explain the motivation behind this change?

Sorry, didn't expect much input on a draft PR yet, so didn't notice the question.

To my knowledge, the motivation is not only it being faster. We already have an interface how we can get data as a stringified JSON without some intermittent layers using the dollar syntax. This is why the expected outcome is that it should be faster, though some testing is soon to be carried out comparing with the current benchmark.

This change would result in a more accurate representation of actual data stored in DB. To illustrate, previously in some cases it seemed to not portray null struct information correctly #itwinjs-backlog/1057; even though code value is unset, previously it was always returned as an empty string. The current getElement output is using EC instance JSON adapters and it seems like it's not really that well extendable and we've had some discussions that ideally we would like to move away from it.

Applications seldom had their own reasons to do the mappings themselves (e.g. iTwin/imodel-transformer in the past). This would streamline mapping by delivering a new mapping function.

I might be missing something, but these are some of the key points. If anything else raises any concerns, please let me know.

[kabentley]

To my knowledge, the motivation is not only it being faster.

I am very skeptical that this change will result in appreciable performance difference - and it's not clear to me which way will be faster. The DgnEelement "cost" you endeavor to eliminate adds caching, which for editing applications (where the same set of elements is read repeatedly) helps tremendously. I'm not sure eliminating it is a good idea at all.

This change would result in a more accurate representation of actual data stored in DB.

If there are bugs in the native code, we should fix them.

Applications seldom had their own reasons to do the mappings themselves (e.g. iTwin/imodel-transformer in the past). This would streamline mapping by delivering a new mapping function.

My strong suggestion is that this change should be implemented in native code, not here. I suppose you can run your tests with this change and see what the profiler says.

[6ar8nas]

/azp run iTwin.js

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pmconne]

more accurate DB data representation

Can you be more specific?

considerably better Elements API performance

Have you measured this?