fix: implement Symbol.hasInstance for cross-library instanceof checks #377
Conversation
|
@kou I’ve fixed the issue. Could you please merge it? |
|
Could you add tests for this? |
kou
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request implements cross-library instanceof checks for Apache Arrow's core types by using Symbol.for() based markers and Symbol.hasInstance. The change addresses a long-standing issue where instanceof checks would fail when multiple versions or instances of the Arrow library were loaded in the same application, particularly affecting integrations with libraries like LanceDB.
Changes:
- Implemented Symbol.hasInstance and static is* methods for Schema, Field, DataType, Data, Vector, RecordBatch, and Table classes to enable reliable instanceof checks across different Arrow library instances
- Added new utility file with helper functions (isArrowSchema, isArrowTable, etc.) for explicit type checking
- Properly exported new functionality through the main Arrow.ts entry point
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/schema.ts | Added Symbol.hasInstance and isSchema/isField methods using global symbols for cross-instance type checking |
| src/type.ts | Added Symbol.hasInstance and isDataType method for DataType class |
| src/data.ts | Added Symbol.hasInstance and isData method for Data class |
| src/vector.ts | Added Symbol.hasInstance and isVector method for Vector class |
| src/recordbatch.ts | Added Symbol.hasInstance and isRecordBatch method for RecordBatch class |
| src/table.ts | Added Symbol.hasInstance and isTable method for Table class |
| src/util/typecheck.ts | New utility file providing exported helper functions for type checking that work across library instances |
| src/Arrow.ts | Added exports for new type checking helper functions and integrated them into the util namespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/schema.ts
Outdated
| /** | ||
| * Check if an object is an instance of Schema. | ||
| * This works across different instances of the Arrow library. | ||
| */ | ||
| static isSchema(x: any): x is Schema { | ||
| return x?.[kSchemaSymbol] === true; | ||
| } | ||
|
|
||
| /** | ||
| * Custom instanceof handler to work across different Arrow library instances. | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance | ||
| */ | ||
| static [Symbol.hasInstance](x: any): x is Schema { | ||
| return Schema.isSchema(x); | ||
| } |
There was a problem hiding this comment.
The new type checking functionality (Symbol.hasInstance and static is* methods) lacks test coverage. Consider adding tests that:
- Verify instanceof works across different library instances using Symbol.hasInstance
- Test the static is* methods (isSchema, isField, isDataType, etc.)
- Test the exported helper functions (isArrowSchema, isArrowField, etc.)
- Verify the behavior when objects don't have the marker symbol
These tests would ensure the cross-library instanceof functionality works as intended and doesn't regress in future changes.
I have added a test for this. |
domoritz
left a comment
domoritz
left a comment
There was a problem hiding this comment.
Very nice.
- I wonder whether we can do some kind of check whether a second instance of arrow exists. We could show a warning. Otherwise I'd worry that we will at some point run into issues where people have multiple versions of Arrow in their systems for a long time.
- What's the performance impact of these changes? This needs to be addressed before we can merge the changes.
| } | ||
|
|
||
| /** @ignore */ | ||
| const kDataSymbol = Symbol.for('apache-arrow/Data'); |
There was a problem hiding this comment.
This is a good solution. We just need to make sure to not accidentally use Symbol() instead of Symbol.for since the former does not create unique instances.
domoritz
left a comment
domoritz
left a comment
There was a problem hiding this comment.
See request for a perf analysis. We have benchmarks in this repo in https://github.com/apache/arrow-js/tree/main/perf.
Great suggestions!
instanceof with vs without Symbol.hasInstance |
|
@domoritz Could you please let me know if any further changes are needed, or if this is ready to merge? |
Divyanshu-s13
force-pushed
the
fix-instanceof-cross-library
branch
from
7bdc11c to
0a3ec76
Compare
|
@kou I’ve fixed the issue. Could you please merge it?I have fixed the CI failure and now it is ready to merge. |
|
I'd like an okay from @trxcllnt |
3 participants
What's Changed
Fixed the instanceof check issue that occurs when multiple versions or instances of the Arrow library are loaded in the same application. Previously, checks like [value instanceof Schema] would fail if the value came from a different Arrow library instance (e.g., when a library like LanceDB uses a different Arrow version than the user's code).
Now instanceof works reliably across different Arrow library instances by using global symbols for type identification.
Also added helper functions like [isArrowSchema()],[isArrowTable()], etc. for explicit type checking.
Closes #61.