Boolean columns #384
Boolean columns #384
Conversation
|
Looks like the e2e tests fail with hash based ordering issues. How to solve? And the linter fails with cognitive complexity: fair enough, it does strike me that the code is missing some abstractions for these things in general. But this will need a bit more study on my part and it's not the right time to do this now. Alternatively could introduce a macro or template here as a hack but I'm not sure the linter will be happier about it? |
There was a problem hiding this comment.
Looks good so far 👍
- We need to update the docs, though not now. I created Docs: Document boolean columns LAPIS#734.
- Should we add unit tests that test the
BooleanEqualsfilter?
|
And please squash your commits before merging such that the message conforms to https://www.conventionalcommits.org/. |
That wasn't actually the case; it didn't get the new column in the test results because the database_config.yaml for the ndjson tests wasn't updated (which I didn't notice because my test runner didn't run the ndjson tests; I'll make a PR with a proper test runner in the future). I have also pushed a commit that replaces exampleDatasetAsNdjson/database_config.yaml with a symlink that points to exampleDataset/database_config.yaml, since the two files are identical. In the future if the files ever diverge, the symlink could be replaced with the changed content again. |
There are 3 e2e tests that do so?: endToEndTests/test/queries/booleanEquals*.json I guess you really mean unit tests? |
Yes, I mean unit tests in https://github.com/GenSpectrum/LAPIS-SILO/tree/003c7ca53cb7915493e1a0871519a397dde77de6/src/silo/test |
pflanze
force-pushed
the
boolean_columns
branch
2 times, most recently
from
9670911
to
a38de21
Compare
I've edited the messages to follow conventional commits, while keeping the commits separate as per discussion; except I've reduced the commits with test updates to only 2. |
pflanze
force-pushed
the
boolean_columns
branch
from
a38de21
to
b294a51
Compare
fengelniederhammer
force-pushed
the
boolean_columns
branch
from
195b1de
to
d431b28
Compare
…mnGroup}::bool_columns fields
- {addValueToColumn,reserveSpaceInColumn,getSubgroup,getValue}()
- Had to bring 'complexity' down to satisfy clangd, hence separate
addNullToColumn() method and early value.IsNull() handling.
…alisation toDatabaseValueType / toString
…nfigReader::readConfig()
- add DatabasePartition::insertColumn( storage::column::BoolColumnPartition ) - update DatabasePartition::validateMetadataColumns
No need for a wrapper, a type alias is enough.
std::string has no `+` overload for string_view, hence use fmt::format
…esults
- Update {exampleDataset,exampleDatasetAsNdjson}/database_config.yaml.
- `small_metadata_set.tsv`: add test_boolean_column column with
manually-randomized choices.
- Add metadata.test_boolean_column to `input_file.ndjson`. Copied over
from `small_metadata_set.tsv` using
https://github.com/pflanze/ndjson-updater/.
- Add new column to test results; sorting changed because it is
currently dependent on the hash of the object contents.
Test results were verified using https://github.com/pflanze/ndjson-updater/. In `booleanEquals_Or.json`, `"value": "B.1"` would return `"count": 97`, but the second subquery would include all of those from the first hence not so interesting, thus `"B.1.1"` was chosen.
…symlink The contents was and likely will remain identical to exampleDataset/database_config.yaml
To satisfy linter.
pflanze
force-pushed
the
boolean_columns
branch
from
0fdb686
to
685db9f
Compare
|
Changed the commit messages to properly follow conventional commit spec, and only using issue number in the last commit to avoid spamming the linked GitHub issue. This concludes the work, if there are no objections I will merge. |
|
I don't know what failed in the CI. It appears to be something building the docker container? The tests run fine for me locally and the content didn't change from before the last force push. |
|
After re-running for about the 4th time both tests went through, so it was an issue with GitHub running out of some resources, or some indeterminism in the tests (race condition). |
resolves #219