alamb commented on PR #1435:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1435#issuecomment-2468945628

   Sorry for my silence / inability to find time to review this PR in depth. I 
am struggling with too many thing
   
   TLDR I I think we should:
   1. Get the tests passing on this PR and merge it in (if not all AST nodes 
have spans initially, we can file tickets to add the information over the next 
few releases)
   2. Add some documentation explaining what is happening and how downstream 
consumers need to change (I think basically by ignoring the newly added fields
   
   @iffyio  and I spoke about this a bit -- basically in my opinion adding 
source locations to sqlparser-rs is one of the last major remaining features it 
is missing.
   
   However, it also has the potential to be very disruptive to downstream 
consumers.
   
   It seems to me the requirements for this change are:
   1. Any changes required by users of this crate should be mechanical
   3. We should clearly document what changes are needed on upgrade (ideally 
with examples)
   
   An example of a mechanical change might be adding
   ```rust
   match node {  
     ast::Query { 
       field1,
       field2, 
       location: _,  // add a new line to ignored location
   }
   ```
   
   An example of a non mechanical change might be wrapping all AST nodes in 
some other struct so the structure of the `match` no longer works
   
   I looked at this PR and I think it basically satisfies 1 for me so I think 
we should proceed. Would it be possible to write up something, perhaps in the 
`README` that explains  that we are adding new spans to the code and how we 
will handle them?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to