-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(ingest/powerbi): support PowerBI parameter references #7523
Conversation
parse_tree: Tree = lark_parser.parse(expression) | ||
# Replace U+00a0 NO-BREAK SPACE with a normal space. | ||
# Sometimes PowerBI returns expressions with this character and it breaks the parser. | ||
expression = expression.replace("\u00a0", " ") |
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.
curious how the heck you found this.. nice!
metadata-ingestion/src/datahub/ingestion/source/powerbi/m_query/parser.py
Show resolved
Hide resolved
logger.debug(f"Actual Value = {actual_value}") | ||
logger.debug(f"Expected Value = {variable}") | ||
if TRACE_POWERBI_MQUERY_PARSER: | ||
logger.debug(f"Actual Value = {actual_value}") |
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.
do we want a higher level here if tracing is enabled?
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.
if they have trace enabled, they probably should have debug too. I expect the trace stuff will only be useful in dev so it should be fine
identifier = node.children[0].children[0] | ||
assert isinstance(identifier, Token) | ||
|
||
ref = identifier.value # ref will have quotes around it. |
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.
Always?
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.
for quoted_identifier yes, but i've added a more detailed comment around this
metadata-ingestion/src/datahub/ingestion/source/powerbi/powerbi.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/powerbi/powerbi.py
Outdated
Show resolved
Hide resolved
aspect=upstream_lineage, | ||
) | ||
mcps.append(mcp) | ||
if len(upstreams) > 0: |
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.
is this unindent intentional? was this just a bug?
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.
it was intentional, fixing a subtle bug where if there were two upstreams then we'd emit two upstreamLineage mcps instead of a single one
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.
oh wow. thanks!
metadata-ingestion/src/datahub/ingestion/source/powerbi/rest_api_wrapper/data_resolver.py
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/powerbi/rest_api_wrapper/data_resolver.py
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/powerbi/rest_api_wrapper/data_resolver.py
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/powerbi/rest_api_wrapper/powerbi_api.py
Show resolved
Hide resolved
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.
LGTM. Minor comments otherwise looks great! Big improvement for other folks using PowerBI.
@@ -426,7 +432,7 @@ each_expression: "each" WS_INLINE each_expression_body | |||
|
|||
each_expression_body: function_body | |||
|
|||
let_expression: "let" NEWLINE WS_INLINE? variable_list WS_INLINE? NEWLINE? in_expression | |||
let_expression: "let" whitespace variable_list WS_INLINE? NEWLINE? in_expression |
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.
is this change safe?
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.
yes - this lets you put whitespace before/after the newline, and also lets you put let Source = ...
on the same line
all of these are valid m-query so we have to match it
Right now this only works for non-admin API. Implementing this for the admin API interface will require additional investigation.
Checklist