Skip to content

Fix handling of unknown types#82

Merged
bdarnell merged 3 commits intocockroachdb:masterfrom
zacps:unknown-type-call
Nov 27, 2019
Merged

Fix handling of unknown types#82
bdarnell merged 3 commits intocockroachdb:masterfrom
zacps:unknown-type-call

Conversation

@zacps
Copy link
Copy Markdown

@zacps zacps commented Apr 25, 2019

When the dialect encounters an unknown type it tries to instantiate
NULLTYPE which is not callable. This removes that and correctly passes
it as a sentinel value.

Essentially an updated version of #31

@zacps
Copy link
Copy Markdown
Author

zacps commented May 8, 2019

The tests passed, build failure appears to be because of a missing dependency in the CI environment.

@ckoehn ckoehn mentioned this pull request Jun 9, 2019
When the dialect encounters an unknown type it tries to instantiate
NULLTYPE which is not callable. This removes that and correctly passes
it as a sentinel value.
@zacps zacps force-pushed the unknown-type-call branch from 4e2e035 to c2a0b4f Compare October 27, 2019 20:58
@zacps
Copy link
Copy Markdown
Author

zacps commented Oct 27, 2019

I just noticed #84, rebased with master.

@bdarnell
Copy link
Copy Markdown
Contributor

bdarnell commented Nov 7, 2019

Thanks for the contribution and for updating the PR! I just have one request (shared with #31 (comment)): can you please add a test that actually executes this line?

Copy link
Copy Markdown
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm just marking this with "requested changes" so that @bdarnell 's comment can get addressed if possible.

Zac Pullar-Strecker added 2 commits November 27, 2019 12:58
This test creates a column with a known type, removes the type from the
type map, then attempts to find the type from the database. This should
return `NULLTYPE`. A warning thrown by `get_columns` has to be suppressed
for the test to pass.
@zacps
Copy link
Copy Markdown
Author

zacps commented Nov 27, 2019

I've added a test, and confirmed that is was failing before the change I made.

Copy link
Copy Markdown
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants