Does bindIdSet in class QueryBinder truly need a OrderedId64Iterable?

[nick4598]

itwinjs-core/core/common/src/ConcurrentQuery.ts

Lines 450 to 467 in c2df697

	/**
	* Bind @type OrderedId64Iterable to ECSQL statement.
	* @param indexOrName Specify parameter index or its name used in ECSQL statement.
	* @param val @type OrderedId64Iterable value to bind to ECSQL statement.
	* @returns @type QueryBinder to allow fluent interface.
	*/
	public bindIdSet(indexOrName: string | number, val: OrderedId64Iterable) {
	this.verify(indexOrName);
	const name = String(indexOrName);
	OrderedId64Iterable.uniqueIterator(val);
	Object.defineProperty(this._args, name, {
	enumerable: true, value: {
	type: QueryParamType.IdSet,
	value: CompressedId64Set.sortAndCompress(OrderedId64Iterable.uniqueIterator(val)),
	},
	});
	return this;
	}

It appears the implementation of bindIdSet which Ive linked above uses sortAndCompress on the passed in val parameter. Wouldn't this make it so that the type for bindIdSet could actually just be Iterable<Id64String>?

I could pass Id64.iterable(id64Arg) without issues?

EDIT: OrderedId64Iterable.uniqueIterator seems to expect an ordered collection of Id64String so I guess I may be wrong. Does that mean the sortAndCompress is redundant though and we could just use CompressedId64Set.compressIds?

[pmconne]

Interested in @khanaffan's feedback but from my perspective we shouldn't rely on or expect the caller to ensure the input is ordered (and if we did expect that, we shouldn't be sorting it again ourselves).
(Also line 459 doesn't do anything).
IMO it should take Iterable<Id64String>.
If there's a desire to optimize for the case when the caller knows the input is already ordered, I'd add a new method that asserts that the input meets that constraint and doesn't re-sort it.

[khanaffan]

I reviewed the original pull request where it was added, and it appears I didn’t add line 459. I assumed that using OrderedId64Iterable would automatically provide sorted elements

https://github.com/iTwin/itwinjs-core/pull/2379/files#diff-4452bb9db888dd7c4cb92a60bbe928831fabf27bdf7691507c3b2ffbebc40ca0R215

In a different pull request, I discovered that wasn’t the case. Instead of changing it to Iterable<Id64String>, I left it as is and called sortAndCompress(). Line 459 was mistakenly left behind.

https://github.com/iTwin/itwinjs-core/pull/2691/files#diff-4452bb9db888dd7c4cb92a60bbe928831fabf27bdf7691507c3b2ffbebc40ca0R226

Now looking at it Iterable<Id64String> look better and probably was my choice except I did not wanted to sort and end up sorting it any way.

[pmconne]

Do bear in mind that string is-a Iterable<string>; you'll probably want to check typeof val === "string" to catch people passing you a single string, otherwise you'll iterate over its individual characters.