You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@texera.apache.org by "AnzhiZhang (via GitHub)" <gi...@apache.org> on 2025/08/17 22:40:55 UTC
[PR] fix: improve equality check for PortSchemas [texera]
AnzhiZhang opened a new pull request, #3667:
URL: https://github.com/apache/texera/pull/3667
fixes #3562
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "AnzhiZhang (via GitHub)" <gi...@apache.org>.
AnzhiZhang commented on code in PR #3667:
URL: https://github.com/apache/texera/pull/3667#discussion_r2296773560
##########
core/gui/src/app/common/util/workflow-compilation-utils.ts:
##########
@@ -30,7 +31,7 @@ export function areAllPortSchemasEqual(schemas: (PortSchema | undefined)[]): boo
if (schemas.length <= 1) {
return true;
}
- return schemas.every(schema => schema === schemas[0]);
+ return schemas.every(schema => isEqual(schemas[0], schema));
Review Comment:
```ts
import { isEqual } from "lodash";
// true
console.log(isEqual(undefined, undefined));
// true
console.log(isEqual(null, null));
// false
console.log(isEqual(undefined, null));
// false true
console.log(isEqual({}, {}));
console.log({} === {});
// false true
console.log({ a: 1 } === { a: 1 });
console.log(isEqual({ a: 1 }, { a: 1 }));
// false false
console.log({ a: 1 } === undefined);
console.log(isEqual({ a: 1 }, undefined));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3667:
URL: https://github.com/apache/texera/pull/3667#discussion_r2281046535
##########
core/gui/src/app/common/util/workflow-compilation-utils.ts:
##########
@@ -30,7 +30,12 @@ export function areAllPortSchemasEqual(schemas: (PortSchema | undefined)[]): boo
if (schemas.length <= 1) {
return true;
}
- return schemas.every(schema => schema === schemas[0]);
+ if (schemas[0] === undefined) {
+ return schemas.every(schema => schema === undefined);
+ } else {
+ const firstSchemaString = JSON.stringify(schemas[0]);
Review Comment:
Using JSON.stringify for equality comparison can be inefficient and unreliable. JSON.stringify doesn't guarantee consistent property ordering, which could cause structurally identical objects to be considered different. Consider using a dedicated deep equality function like lodash's isEqual or implementing a proper equality method for PortSchema objects.
##########
core/gui/src/app/common/util/workflow-compilation-utils.ts:
##########
@@ -30,7 +30,12 @@ export function areAllPortSchemasEqual(schemas: (PortSchema | undefined)[]): boo
if (schemas.length <= 1) {
return true;
}
- return schemas.every(schema => schema === schemas[0]);
+ if (schemas[0] === undefined) {
+ return schemas.every(schema => schema === undefined);
+ } else {
+ const firstSchemaString = JSON.stringify(schemas[0]);
+ return schemas.every(schema => schema !== undefined && JSON.stringify(schema) === firstSchemaString);
Review Comment:
JSON.stringify is called for every schema in the array, which is inefficient for large arrays. The stringification of the first schema should be done once (which is correct), but calling JSON.stringify in the loop for each comparison is wasteful. Consider caching the stringified versions or using a more efficient comparison method.
```suggestion
const schemaStrings = schemas.map(schema => schema === undefined ? undefined : JSON.stringify(schema));
return schemaStrings.every(str => str === firstSchemaString);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3667:
URL: https://github.com/apache/texera/pull/3667#discussion_r2281084908
##########
core/gui/src/app/common/util/workflow-compilation-utils.ts:
##########
@@ -30,7 +31,7 @@ export function areAllPortSchemasEqual(schemas: (PortSchema | undefined)[]): boo
if (schemas.length <= 1) {
return true;
}
- return schemas.every(schema => schema === schemas[0]);
+ return schemas.every(schema => isEqual(schemas[0], schema));
Review Comment:
The function doesn't handle `undefined` values properly. If `schemas[0]` is undefined, `isEqual(undefined, schema)` will return true only when `schema` is also undefined, but the function should handle mixed undefined/defined scenarios consistently.
```suggestion
// If all schemas are undefined, return true
if (schemas.every(schema => schema === undefined)) {
return true;
}
// If some schemas are undefined and some are defined, return false
if (schemas.some(schema => schema === undefined) && schemas.some(schema => schema !== undefined)) {
return false;
}
// All schemas are defined, check if all are equal
const firstDefined = schemas.find(schema => schema !== undefined);
return schemas.every(schema => isEqual(firstDefined, schema));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "chenlica (via GitHub)" <gi...@apache.org>.
chenlica commented on PR #3667:
URL: https://github.com/apache/texera/pull/3667#issuecomment-3218333943
@aicam You can be the main reviewer. @aglinxinyuan can be the secondary reviewer.
@AnzhiZhang Please merge the main branch to your branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "AnzhiZhang (via GitHub)" <gi...@apache.org>.
AnzhiZhang commented on code in PR #3667:
URL: https://github.com/apache/texera/pull/3667#discussion_r2281085975
##########
core/gui/src/app/common/util/workflow-compilation-utils.ts:
##########
@@ -30,7 +31,7 @@ export function areAllPortSchemasEqual(schemas: (PortSchema | undefined)[]): boo
if (schemas.length <= 1) {
return true;
}
- return schemas.every(schema => schema === schemas[0]);
+ return schemas.every(schema => isEqual(schemas[0], schema));
Review Comment:
We want to ensure every schema is the same, including `undefined`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam merged PR #3667:
URL: https://github.com/apache/texera/pull/3667
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve equality check for PortSchemas [texera]
Posted by "AnzhiZhang (via GitHub)" <gi...@apache.org>.
AnzhiZhang commented on code in PR #3667:
URL: https://github.com/apache/texera/pull/3667#discussion_r2296773560
##########
core/gui/src/app/common/util/workflow-compilation-utils.ts:
##########
@@ -30,7 +31,7 @@ export function areAllPortSchemasEqual(schemas: (PortSchema | undefined)[]): boo
if (schemas.length <= 1) {
return true;
}
- return schemas.every(schema => schema === schemas[0]);
+ return schemas.every(schema => isEqual(schemas[0], schema));
Review Comment:
```ts
import { isEqual } from "lodash";
// true
console.log(isEqual(undefined, undefined));
// true
console.log(isEqual(null, null));
// false
console.log(isEqual(undefined, null));
// false true
console.log({} === {});
console.log(isEqual({}, {}));
// false true
console.log({ a: 1 } === { a: 1 });
console.log(isEqual({ a: 1 }, { a: 1 }));
// false false
console.log({ a: 1 } === undefined);
console.log(isEqual({ a: 1 }, undefined));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org