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

Depict null values in array type ECProperty output #6948

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e410337
Update the test fixture to be more multi-use applicable
6ar8nas Jul 9, 2024
3966842
use deep equal
6ar8nas Jul 9, 2024
ce45610
append ecsql null behavior docs samples
6ar8nas Jul 10, 2024
64ad57c
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 10, 2024
fc38873
add struct array type to the docs sample
6ar8nas Jul 10, 2024
de7cd0d
Merge branch 'Sarunas/array-null-values' of https://github.com/iTwin/…
6ar8nas Jul 10, 2024
ce28366
Remove docs parts about all of the properties of the struct/array to …
6ar8nas Jul 10, 2024
9df8973
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 11, 2024
0824bc7
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 12, 2024
d32968e
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 15, 2024
13735f1
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 16, 2024
9c9d405
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 17, 2024
c829786
rush change
6ar8nas Jul 17, 2024
5469049
Merge branch 'Sarunas/array-null-values' of https://github.com/iTwin/…
6ar8nas Jul 17, 2024
449776f
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 23, 2024
8a56abb
Bump test schema version to latest
6ar8nas Jul 24, 2024
c085589
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 24, 2024
8221e1b
Remove only on a test
6ar8nas Jul 24, 2024
20a09bb
rename bim file
6ar8nas Jul 24, 2024
8bf77e7
Merge branch 'master' into Sarunas/array-null-values
6ar8nas Jul 25, 2024
0bc48b3
Merge remote-tracking branch 'origin/master' into Sarunas/array-null-…
soham-bentley Dec 19, 2024
7f8a573
Merge remote-tracking branch 'origin/master' into Sarunas/array-null-…
soham-bentley Jan 9, 2025
c2ef552
Merge remote-tracking branch 'origin/master' into Sarunas/array-null-…
soham-bentley Jan 10, 2025
e3f1f21
Merge remote-tracking branch 'origin/master' into Sarunas/array-null-…
soham-bentley Jan 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,24 @@
*--------------------------------------------------------------------------------------------*/
import { assert, expect } from "chai";
import { Id64, Id64String } from "@itwin/core-bentley";
import {
BriefcaseIdValue, Code, ColorDef, GeometricElementProps, IModel,
SubCategoryAppearance,
} from "@itwin/core-common";
import { _nativeDb, IModelDb, IModelJsFs, SnapshotDb, SpatialCategory } from "../../core-backend";
import { Code, ColorDef, GeometricElementProps, IModel, SubCategoryAppearance } from "@itwin/core-common";
import { IModelDb, IModelJsFs, SnapshotDb, SpatialCategory } from "../../core-backend";
import { IModelTestUtils } from "../IModelTestUtils";

interface TestSchemaLocation {
city: string;
zip: number;
}

interface TestElement extends GeometricElementProps {
addresses: [null, {city: "Pune", zip: 28}];
addresses: TestSchemaLocation[];
favoriteNumbers: number[];
}

function initElemProps( _iModelName: IModelDb, modId: Id64String, catId: Id64String, autoHandledProp: any): GeometricElementProps {
// Create props
const elementProps: GeometricElementProps = {
classFullName: "Test:Foo",
classFullName: "Test:TestClass",
model: modId,
category: catId,
code: Code.createEmpty(),
Expand All @@ -28,36 +31,33 @@ function initElemProps( _iModelName: IModelDb, modId: Id64String, catId: Id64Str
return elementProps;
}

describe("Insert Null elements in Struct Array, and ensure they are returned while querying rows", () => {
describe("Various ECProperties null behavior handling cases test fixture", () => {
const testSchema = `<?xml version="1.0" encoding="UTF-8"?>
<ECSchema schemaName="Test" alias="test" version="01.00.00" xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.1">
<ECSchemaReference name="BisCore" version="01.00.04" alias="bis"/>
<ECStructClass typeName="Location" modifier="Sealed">
<ECProperty propertyName="City" typeName="string"/>
<ECProperty propertyName="Zip" typeName="int"/>
</ECStructClass>
<ECEntityClass typeName="Foo" modifier="Sealed">
<BaseClass>bis:PhysicalElement</BaseClass>
<ECArrayProperty propertyName="I_Array" typeName="int"/>
<ECArrayProperty propertyName="Dt_Array" typeName="dateTime"/>
<ECStructArrayProperty propertyName="Addresses" typeName="Location"/>
</ECEntityClass>
<ECSchema schemaName="TestSchema" alias="test" version="01.00.00" xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.1">
<ECSchemaReference name="BisCore" version="01.00.04" alias="bis"/>
<ECStructClass typeName="Location" modifier="None">
<ECProperty propertyName="City" typeName="string"/>
<ECProperty propertyName="Zip" typeName="int"/>
</ECStructClass>
<ECEntityClass typeName="TestClass" modifier="None">
<BaseClass>bis:PhysicalElement</BaseClass>
<ECArrayProperty propertyName="FavoriteNumbers" typeName="int"/>
<ECStructArrayProperty propertyName="Addresses" typeName="Location"/>
</ECEntityClass>
</ECSchema>`;

const schemaFileName = "NullStructElementTest.01.00.00.xml";
const iModelFileName = "NullStructElementTest.bim";
const categoryName = "NullStructElement";
const subDirName = "NullStructElement";
const schemaFileName = "NullBehaviorsTest.01.00.00.xml";
const iModelFileName = "NullBehaviorsTest.bim";
const subDirName = "NullBehaviors";
const iModelPath = IModelTestUtils.prepareOutputFile(subDirName, iModelFileName);
const categoryName = "NullBehaviorsCategory";

before(async () => {
// write schema to disk as we do not have api to import xml directly
const testSchemaPath = IModelTestUtils.prepareOutputFile(subDirName, schemaFileName);
IModelJsFs.writeFileSync(testSchemaPath, testSchema);

const imodel = SnapshotDb.createEmpty(iModelPath, { rootSubject: { name: "InsertNullStructArrayTest" } });
const imodel = SnapshotDb.createEmpty(iModelPath, { rootSubject: { name: "NullBehaviorsTest" } });
await imodel.importSchemas([testSchemaPath]);
imodel[_nativeDb].resetBriefcaseId(BriefcaseIdValue.Unassigned);
IModelTestUtils.createAndInsertPhysicalPartitionAndModel(imodel, Code.createEmpty(), true);

let spatialCategoryId = SpatialCategory.queryCategoryIdByName(imodel, IModel.dictionaryId, categoryName);
Expand All @@ -69,29 +69,30 @@ describe("Insert Null elements in Struct Array, and ensure they are returned whi
imodel.close();
});

it("Test for struct array to contain null structs", async () => {
const testFileName = IModelTestUtils.prepareOutputFile(subDirName, "roundtrip_correct_data.bim");
it("validates arrays to contain null values", async () => {
const testFileName = IModelTestUtils.prepareOutputFile(subDirName, "struct_array_contain_nulls.bim");
const imodel = IModelTestUtils.createSnapshotFromSeed(testFileName, iModelPath);
const spatialCategoryId = SpatialCategory.queryCategoryIdByName(imodel, IModel.dictionaryId, categoryName);
const spatialCategoryId = SpatialCategory.queryCategoryIdByName(imodel, IModel.dictionaryId, categoryName)!;
const [, newModelId] = IModelTestUtils.createAndInsertPhysicalPartitionAndModel(imodel, Code.createEmpty(), true);

// create element with auto handled properties
const expectedValue = initElemProps( imodel, newModelId, spatialCategoryId!, {
addresses: [null, {city: "Pune", zip: 28}],
const expectedProps = initElemProps(imodel, newModelId, spatialCategoryId, {
addresses: [null, { city: "Pune", zip: 28 }],
favoriteNumbers: [1, 44, 31, null, 81, 19],
}) as TestElement;

// insert a element
const geomElement = imodel.elements.createElement(expectedValue);
// Insert an element containing a struct array with at least one null value
const geomElement = imodel.elements.createElement(expectedProps);
const id = imodel.elements.insertElement(geomElement.toJSON());
assert.isTrue(Id64.isValidId64(id), "insert worked");
imodel.saveChanges();

// verify inserted element properties
// Verify the properties of the inserted element
const actualValue = imodel.elements.getElementProps<TestElement>(id);
expect(actualValue.addresses.length).to.equal(2);
expect(actualValue.addresses[0]).to.be.empty;
expect(actualValue.addresses).to.deep.equal([undefined, { city: "Pune", zip: 28 }]);
expect(actualValue.favoriteNumbers.length).to.equal(6);
expect(actualValue.favoriteNumbers).to.deep.equal([1, 44, 31, undefined, 81, 19]);

imodel.close();
});

});
7 changes: 6 additions & 1 deletion docs/learning/ECSQLNullBehaviors.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Documentation schema sample:
<ECSchemaReference name="CoreCustomAttributes" version="01.00.03" alias="CoreCA"/>
<ECEntityClass typeName="DocsElement" modifier="None">
<ECProperty propertyName="intProp" typeName="int"/>
<ECProperty propertyName="p2dProp" typeName="point2d"/>
<ECArrayProperty propertyName="arrBoolProp" typeName="boolean" minOccurs="0" maxOccurs="unbounded"/>
<ECStructProperty propertyName="structProp" typeName="StructType"/>
<ECStructArrayProperty propertyName="arrStructProp" typeName="StructType" minOccurs="0" maxOccurs="unbounded"/>
Expand All @@ -27,7 +28,11 @@ Attempting to update any of the properties (`intProp`, `arrBoolProp`, `structPro

## Setting some complex property children to null

To be added
This section would cover partially setting a complex property (point2d, struct, array) to null.

- A `point2d` type property must always have all its coordinates present, hence updating a point2d to be partially null is not a valid input and would result in the property not being updated. This is why updating `p2dProp` to `{ x: null, y: 2.5 }` would be invalid and take no effect on the element's property.
- A `struct` type property can have all of their children properties set to null. The child property would end up being cleared from the parent object with the remaining properties remaining intact. This means that if `structProp` would include `{ doubleProp: 41.0, stringProp: null }`, the string property would be cleared with the doubleProp successfully updated to the expected value.
- An `array` type (primitive or struct) property can store null values as any other value of the respective type. It would happen to be represented alongside the ordinary properties as null. E.g. updating `arrBoolProp` to `[false, true, null, false]` is a perfectly valid data entry for a boolean type array property.
Copy link
Member

@ColinKerr ColinKerr Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior that has been controversial. @pmconne

There are three possible behaviors:
1 - accept the array as input and persist including the nulls.
2 - consider the array invalid if it has any null inputs.
3 - silently prune the null entries in the array, e.g. persist [false, true, false]

I do not think it's acceptable to prune the nulls from the array so only options 1 and 2 are OK.

I think #1 is the best option but I could be convinced to accept #2 if we're sure no one will ever want to be able to have a sparse array or have a fixed length array where the use null to indicate that an entry is not yet set.

Copy link
Member

@pmconne pmconne Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we permit the possibility of sparse arrays then everywhere we define a TypeScript interface representing any array, we must define it as Array<T | undefined>~ instead of T[]`. We certainly don't do that today, and I don't think that it would make anyone's life better.

If sparse arrays became a requirement at some point, couldn't we consider a custom attribute permitting null entries, similar to how we have IsNullable for primitive properties?

Copy link
Contributor Author

@6ar8nas 6ar8nas Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, these changes only solve the fact that if nulls are already present in the DB, we display the data accurately as we have it. The sparse arrays have already been possible, it's just that we have been using the 3rd behavior that Colin has described above for representation, while storing the data into the respective database columns as has been input for us including all of the nulls and that seems like a bug.

we must define it as Array<T | undefined> instead of T[]

Is it a must to match all of the TypeScript interfaces to what exactly can be stored on the native side immediately? Given the business justification some time in the future, the only needed change would be switching up the interfaces on the TS side. If the interfaces are kept as is and everyone were strict about using them, this behavior should not ever be hit (though iTwin/itwinjs-backlog#1063 suggests it's not as uncommon) and the nulls wouldn't ever be persisted in the database, making this whole problem redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over a discussion, we agreed to run a SQL script through iQuacs to count null data entries in array properties, thus determining whether there is actual usage of sparse arrays. If there is any, it was agreed to move forward with these changes and fix the currently undocumented null stripping from output, and if not - throw an error on finding a null inside an array.


## Setting all complex property children to null or making it empty

Expand Down
Loading