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

adding dshape parameter to CSV #476

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

ywang007
Copy link
Contributor

@ywang007 ywang007 commented Aug 16, 2016

This resolves part of #461 issue, so that when dshape is provided, it is taken by CSV.
However, this does not resolve the root of data type mismatch in #461.

@ywang007 ywang007 changed the title adding dshape parameter to CSV [WIP] adding dshape parameter to CSV Aug 16, 2016
@ywang007 ywang007 changed the title [WIP] adding dshape parameter to CSV adding dshape parameter to CSV Aug 16, 2016
# explicitly test that we do NOT preserve the header here
assert discover(csv).measure.names != discover(sql).measure.names
# explicitly test that we do NOT preserve the header here ???
#assert discover(csv).measure.name != discover(sql).measure.name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason the header should NOT be preserved? Or was it just not preserved before because we kept rediscovering the datashape?

Copy link
Member

Choose a reason for hiding this comment

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

Why was this commented out?

@ywang007
Copy link
Contributor Author

@kwmsmith, since we are adding the datashape parameter for a very specific purpose, that is to allow user force one when he's not satisfied with the discovered one, I figure it's better to use a different variable name, user_dshape, to make it more explicit, and avoid conflicts with any internal use of "dshape" parameter by odo. This will require us to update blaze doc accordingly.

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

Successfully merging this pull request may close these issues.

2 participants