Fix MongoStatement.getFieldNamesFromProjectStage
#72
Conversation
stIncMale
force-pushed
the
fixProjectStageParsing
branch
from
37c1be5 to
85ddbd8
Compare
|
Both issues addressed in this PR were discovered by @NathanQingyangXu. |
src/test/java/com/mongodb/hibernate/jdbc/MongoStatementTests.java
Outdated
Show resolved
Hide resolved
| if (exclude && !key.equals(ID_FIELD_NAME)) { | ||
| throw new RuntimeException(format( | ||
| "Exclusions are not allowed in `$project` specifications, except for the [%s] field: [%s, %s]", | ||
| ID_FIELD_NAME, key, value)); |
There was a problem hiding this comment.
This is hardly to ever change, unless we start requiring a schema or start inferring it.
There was a problem hiding this comment.
well, it is native query related. Currently let us keep it in mind and then move on.
There was a problem hiding this comment.
Could you please elaborate on why this is related to native queries?
There was a problem hiding this comment.
because for non-native query we have 100% control of the MQL generation, including the $project stage. If we saw no need to use exclusion list (from my understanding, explicit inclusion list suffices and should be relied upon for SQL AST provides all the list already) during MQL translation, this issue won't bother us at all.
There was a problem hiding this comment.
That still does not explain your "well, it is native query related" comment. Do you think native queries will have to use exclustions? If they do and we allow that, how can we possibly know the list of the columns for ResultSet without inferring that list from the returned documents, which we don't have plans to do?
There was a problem hiding this comment.
Sorry to jump in here. The way I understand it, native queries will not have to use exclusions. The question is whether we allow them to. I also can't see a way to make it work, so I don't think we will allow them to.
There was a problem hiding this comment.
Right. And if we are not going to allow them to use exclusions, then the code this thread is about will likely not change, which is what I said in the first clarifying comment for reviewers. And that makes me wonder, what is it that we need to keep in mind in relation to this code and native queries (I am trying to understand the comment #72 (comment), assuming it expressed or implied something that I am missing).
There was a problem hiding this comment.
If we made it a hard rule that native query won't use exclusion list, then no need to keep that in mind? My point is there are many edge cases for native query and this might be another one; but if we forbid it, it becomes eaiser.
There was a problem hiding this comment.
That still does not explain your "well, it is native query related" comment. Do you think native queries will have to use exclustions? If they do and we allow that, how can we possibly know the list of the columns for
ResultSetwithout inferring that list from the returned documents, which we don't have plans to do?
I didn't imply that. Sorry for misunderstanding. I meant the exclusion list could only happen in theory in native query. Native query has many edge cases and this is another one. We all agree exclusion list is hard to our proudct.
| } | ||
| if (!value.isBoolean() && !value.isInt32()) { | ||
| throw new FeatureNotSupportedException(format( | ||
| "Expressions and literals are not supported in `$project` specifications: [%s: %s]", key, value)); |
There was a problem hiding this comment.
This is likely to change. When it does, we should try to allow only the bare minimum we need or know works (for example, because we have tests), and keep forbidding the rest.
No description provided.