-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
WIP: Add SampleId as a main name for sample id column in a column description file #655
Conversation
please agree to cla, see examples in other pr-s |
@arcadia-devtools Ship it! |
Please add a test that shows that both DocId and SampleId work |
@@ -70,6 +70,9 @@ namespace { | |||
if (type == "Target") { | |||
type = "Label"; | |||
} | |||
if (type == "DocId") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same logic should be added where output columns names are parsed in eval_result.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it is tricky. I was expected that column_descriptor/
is the only place where string names of column types are used. I think it would be better to pass TVector<TColumn>
instead of TVector<TString>
to OutputEvalResultToFile()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to pass TVector instead of TVector to OutputEvalResultToFile().
I agree but this is more involved refactoring outside of this particular change.
@annaveronika, internal review request created: 701585 |
@arcadia-devtools Ship it! |
@annaveronika, internal review request created: 709727 |
@daskol You need to agree to our cla (see examples in other pr-s), otherwise we will not be able to merge your pr. |
@daskol Do you plan to proceed with the pr? |
@arcadia-devtools Ship it! |
@andrey-khropov, internal review request created: 737216 |
…ell. Canonize tests
…ell. Canonize tests
No description provided.