Skip to content

DRILL-3920 Add vector loading tests#194

Closed
cwestin wants to merge 1 commit intoapache:masterfrom
cwestin:alloc-vector-test
Closed

DRILL-3920 Add vector loading tests#194
cwestin wants to merge 1 commit intoapache:masterfrom
cwestin:alloc-vector-test

Conversation

@cwestin
Copy link
Contributor

@cwestin cwestin commented Oct 9, 2015

Additional tests added to TestValueVectors for serialization and loading; these tests were used to find and debug issues with DrillBuf slicing. Some light cleanup of a few vector implementations.

Unit tests pass.
Regression suite has a few (known recurring) failures:

  • timeouts while waiting to send intermediate work fragments
  • timeouts for "Fetch parquet metadata" tasks not complete
  • Selected column 'dir0' must have name 'columns' or must be plain '*'

@jacques-n
Copy link
Contributor

Can you break each of these commits into its own jira? It is confusing that you are doing a bunch of independent commits under the same jira number.

@cwestin
Copy link
Contributor Author

cwestin commented Oct 11, 2015

@cwestin cwestin changed the title DRILL-1942-vector-test DRILL-3920 Add vector loading tests Oct 11, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

the logic in clear() seems duplicating close(). we should directly call close perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But clear() doesn't call vectors.clear(). I seem to recall a problem with adding that to clear() in the past. I'm worried this will introduce a regression, so I'd rather not take that change on now.

Additional tests added to TestValueVectors for serialization and loading.
Some light cleanup of a few vector implementations.
@cwestin
Copy link
Contributor Author

cwestin commented Oct 13, 2015

I made the changes indicated above, but not any that might destabilize tests (noted above). Please re-review and merge.

@hnfgns
Copy link
Contributor

hnfgns commented Oct 13, 2015

+1

@asfgit asfgit closed this in 2736412 Oct 13, 2015
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