Upgrade com.puppycrawl.tools:checkstyle 8.41 -> 8.41.1

This PR contains the following updates:

Package
Update
Change

com.puppycrawl.tools:checkstyle (source)
patch
8.41 -> 8.41.1

[ ] If you want to rebase/retry this PR, check this box

Suggested commit message:
Upgrade Checkstyle 8.41 -> 8.41.1

See:
- https://checkstyle.sourceforge.io/releasenotes.html
- https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-8.41.1
- https://github.com/checkstyle/checkstyle/compare/checkstyle-8.41...checkstyle-8.41.1


Broken dependencies show as 'present'

DO NOT REMOVE OR SKIP THE ISSUE TEMPLATE

[X] I understand that I will be blocked if I remove or skip any mandatory* field

Checklist

[X] I'm reporting a bug unrelated to a specific site
[X] I've verified that I'm running yt-dlp version 2022.11.11 (update instructions) or later (specify commit)
[X] I've checked that all provided URLs are playable in a browser with the same IP and same login details
[X] I've checked that all URLs and arguments with special characters are properly quoted or escaped
[X] I've searched the bugtracker for similar issues including closed ones. DO NOT post duplicates
[X] I've read the guidelines for opening an issue

Provide a description that is worded well enough to be understood

I've verified that I'm running yt-dlp version 2022.11.11 (update instructions) Git HEAD d7b460d0e

I've checked that all provided URLs are playable in a browser with the same IP and same login details N/A

The function _get_exe_version_output() used in checking the version of dependencies like ffprobe doesn't check whether running the program failed.
In the test below I aliased ffprobe to wc, the POSIX word count program, which returns error 1 when invoked as wc -version. Even though the supposed ffprobe program couldn't respond to -version, its output was considered valid and led to the program being listed as present.
This is a possible improvement where the string **broken** would be shown instead of present in such a case, but doubtless there are other resolutions that would be as valid or better.
--- old/yt_dlp/utils.py
+++ new/yt_dlp/utils.py
@@ -2703,8 +2703,10 @@
         # STDIN should be redirected too. On UNIX-like systems, ffmpeg triggers
         # SIGTTOU if yt-dlp is run in the background.
         # See https://github.com/ytdl-org/youtube-dl/issues/955#issuecomment-209789656
-        stdout, _, _ = Popen.run([encodeArgument(exe)] + args, text=True,
+        stdout, _, ret = Popen.run([encodeArgument(exe)] + args, text=True,
                                  stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+        if ret != 0:
+            return None
     except OSError:
         return False
     return stdout
@@ -2726,6 +2728,8 @@
     """ Returns the version of the specified executable,
     or False if the executable is not present """
     out = _get_exe_version_output(exe, args)
+    if out is None:
+        return '**broken**'
     return detect_exe_version(out, version_re, unrecognized) if out else False

For a real-life example of this in yt-dl see https://github.com/ytdl-org/youtube-dl/issues/31355.
Provide verbose output that clearly demonstrates the problem

[X] Run your yt-dlp command with -vU flag added (yt-dlp -vU <your command line>)
[X] Copy the WHOLE output (starting with [debug] Command-line config) and insert it below: showing only debug output for the issue

Complete Verbose Output
$ ln -s $(which ffmpeg) ./ffmpeg
$ ln -s $(which wc) ./ffprobe
$ python3.9 -m yt_dlp -v -F 'https://drive.google.com/file/d/1TJlxla4cNxKseUTZ7OKKJmE4ct2dxQSY/view?usp=drivesdk' --ffmpeg-location  .
[debug] Command-line config: ['-v', '-F', 'https://drive.google.com/file/d/1TJlxla4cNxKseUTZ7OKKJmE4ct2dxQSY/view?usp=drivesdk', '--ffmpeg-location', '.']
[debug] Encodings: locale UTF-8, fs utf-8, pref UTF-8, out utf-8, error utf-8, screen utf-8
[debug] yt-dlp version 2022.11.11 [8b644025b] (source)
[debug] Lazy loading extractors is disabled
[debug] Plugins: ['SamplePluginIE', 'SamplePluginPP']
[debug] Git HEAD: d7b460d0e
[debug] Python 3.9.15 (CPython i686 32bit) - Linux-4.4.0-210-generic-i686-with-glibc2.23 (OpenSSL 1.1.1s  1 Nov 2022, glibc 2.23)
[debug] exe versions: ffmpeg 4.3, ffprobe present
[debug] Optional libraries: Cryptodome-3.11.0, certifi-2019.11.28, secretstorage-3.2.0, sqlite3-2.6.0
[debug] Proxy map: {}
[debug] Loaded 1725 extractors
...

So is this a bug that will get fixed or do I need to try fixing the ffprobe installation?

Response in the yt-dl thread.

The trick both here and in yt-dl is the logic whereby the broken program both gets listed as broken and is marked as not available.


How to force url_launcher package to launch a url in a specific browser?

Use case
I am using a payment getaway that forces me to open their hosted page in a specific browser (Safari for ios) and (chrome for android) , i worked around this issue and launched the url in the default browser but sometimes the default browser wouldn't be safari or chrome.
Proposal
I would like to bet an option with url_package to launch urls in specific browser

@mo7amedaliEbaid
Check if this helps in your case or not.

Controlling specific application launch is out of scope for url_launcher; its purpose is simply to hand off a URL to the system for the system to do standard handling with. There are two potential options here:

Use url_launcher as-is, using a browser-specific scheme (e.g., on some versions of iOS, Safari handles x-safari-https: as a scheme, apparently) to control the OS-level routing.
Write or find a plugin that uses a different system for launching that is app-based, rather than standard-OS-API based. Note that on iOS it's likely that directly selecting an application is impossible due to OS restrictions.


T611: Connected subspaces of the reals are contractible

Add T611: embeddable in $\mathbb R$ + connected + not empty ==> contractible.
And cleanup two redundant trait files.

I'm not sure if my comment was noticed, but 611 is a trivial theorem as is.

I'm not sure if my comment was noticed, but T611 is a trivial theorem as is.

No, I did not see your comment. Where?

I must have done the review wrong. But around noon today I started a review and made the following inline comment
"Change " - P000137: true" to " - P000137: false".

Ah, good catch.  I'll do another fix and then maybe you can approve that one.

Thanks @danflapjax


Coinb.in and blockr show no inputs

Hello,
I traded with a partner and used coinb.in multi-sig.
The redeem script
522102ac174028778912dac983b886cf87a242e44e07d5cbdbab214f03e9a83616f840210276ae95c64f264f78391fe0799882bea6edfd66650c44803a5c5462f50a3919a921020e02f631816560e646d23486ad3a210b452f5b41b6d8fd0814db4a67147f605d53ae
has been generated and shows the following wallet:
3P2jxfzHTjxvq29P85nGddNKqUkcXfeiYi
The trade has been successfull and we wanted now to create a transaction using the redeem script above.
Unfortunately, after loading the redeem script, there are no inputs and no outpots shown (both on 0.00000).
The linked wallet (https://btc.blockr.io/address/info/3P2jxfzHTjxvq29P85nGddNKqUkcXfeiYi) shows 0.000 BTC as well.
BUT if I check any other sites like blockchain, chain.so etc., the wallet shows confirmed inputs:
https://blockchain.info/address/3P2jxfzHTjxvq29P85nGddNKqUkcXfeiYi
I was  transferring like 0.0002166 BTC to check if it helps, too but even they didn't show up.
How can I get my money now?
Thanks for help. Leave your wallet for donations in case of successfull!

coinb.in by default uses blockr
It looks like a bug with blockr.
Try reporting the bug to them.

I did - but the last blockr.io post on facebook had been 2015 and twitter 3 months ago.
To my bug report, they haven't responded yet.
If they won't react, my money gone? What else could I do?

iirc you can change the site you use in coinb.in's settings

Nope, unfortunately not: With Bitcoin there is only blockr.io as far as I can see :(

Dude, I have exactly the same problem. Can anybody help? Also me, I'm willing to donate for help

Hi there,
The bitcoin demon running had failed and this had gone unnoticed, when it fails our system will attempt to retrieve unspent outputs and balances from blockr.io, but clearly they are having issues to.
As of now, we have a second node running on a separate server which should give more stability. Further more should both of our nodes fail we will use blockr.io and blockchain.info to compare and return more accurate balances.
If anyone has any other problems, please feel free to get in touch.


[JOSS 6532] Relationship to JAX's built in "external callback" functionality

If I understand the implementation properly (although the docs are not very clear about the implementation details), this library works by acquiring the Python GIL and executing a Python callback from within the XLA custom call. As far as I can tell, this is the same behavior offered by JAX's built in external callback functionality, which can be combined with the jax.custom_jvp and/or jax.custom_vjp decorators to achieve the same results as JAXBind. I'd be interested to understand the difference between these two approaches, and a clear comparison should be added to the docs (and JOSS paper).
As a smaller side note, it might be also interesting to compare to the Pallas submodule, which takes a slightly different approach to similar ends on GPUs and TPUs.
ref: https://github.com/openjournals/joss-reviews/issues/6532

I'm not sure I totally follow the argument about how crucial it is to define both a JVP and VJP. I think that the "linear" point would also hold for an implementation using custom_jvp?
Regardless, I think it would be great for the docs to make a very clear case about this comparison!
Side note: as far as I can tell, it's not too hard to define a custom primitive that uses the external callback mechanism under the hood (e.g. lowering with mlir.lower_fun), so it's possible that the compiled component of JAXBind could be replaced by jax.pure_callback, while keeping the same front end. This could potentially reduce the maintenance burden.

For the kind of optimizations we usually do with our models (arxiv:1901.11033 and arxiv:2105.10470), we need both the JVP and the VJP. Together they serve as a positive semidefinite estimator of the local "curvature" which we use in a second order optimization.
Unfortunately, custom_jvp is not enough. It allows you to define a custom JVP but you must rely on JAX automatically deriving the VJP for you then. For example, say you want to wrap a host-callback to the FFT, then you can define the JVP to be the FFT itself but you can not define the VJP anymore. JAX will try to derive it for you by transposing the JVP but will fail because it can not transpose a host-callback. Custom transpositions in JAX would solve exactly this problem but so far they have various limitations that make them unsuitable for the types of problems we wanted to solve (see e.g. https://github.com/google/jax/issues/13298 and https://github.com/google/jax/issues/13283).
Very interesting idea! We started with C++ as we were  wrapping individual C++ functions before generalizing the code. Looking back at it, we could probably replace all of the C++ code with something along the lines of this https://github.com/google/jax/discussions/17875 for jax.pure_callback. Having said that, we haven't internally settled the debate yet whether we might want to expose a version of JAXbind free of any python GIL for C/C++ which would require our custom C++ backend.

That all makes sense, thanks! Wrt the JOSS review, I'd just say that all this should be discussed clearly in the docs. Otherwise all sounds good.

Another follow up on this. The paper has a paragraph that starts "To the best of our knowledge no other code currently exists for connecting generic functions to JAX. ..." I'd argue that this is no longer true given this discussion here. In particular, the JAX core library provides an interface for this! JAXbind does provide some extra features, but I think this should be clarified in the text.

Thanks for the comment!
It might be that you looked at an old version of the paper. Unfortunately, I forgot to update the PDF in the JOSS review issue. Most of the changes were already incorporated into the paper. Additionally, I have now updated the summary section of the paper with PR #22.
Let me know if you believe the relation to the external callback functionally is still not clear in the paper.


Responsive(mobile) view & main stage actions-possible?

I have a link on my side panel that is a main stage action to replace the map on the main stage with a PDF document describing the map/project. It looks good in the main view but when I test on an Ipad-sized view, the link on the side panel (which is now the 'learn more' content..would love to change that language but thats another story) does not go to the PDF but rather just back to the map. Is there any way for the responsive design to get to this content? Bug? limitation?
Alternatively, I have put the PDF in a iframe on the side panel-- that could work if I could get the 'fit to width' to be the default view and/or have the iframe code I enter be honored. I try and add standard iframe code I've used elsewhere defining width, height & scrolling, but the tags are ignored and it defaults to its own dimensions. ideas? thx

I am not able to reproduce the first issue. For me the PDF open as expected using Safari on iOS 9.1. But your issue may be that if you don't click exactly on the link, it will get back to the map as this is the expected behavior. Could you try increasing the font size and see if it helps? If that's your issue, you may like the following article to create nice button https://medium.com/story-maps-developers-corner/create-buttons-for-main-stage-actions-using-the-style-tag-144b3aeb5583#.wa4x9pefq
Inline PDF seems to works fine for me, see my test app http://story.maps.arcgis.com/apps/MapJournal/?appid=d56ce09c17624eb2a7d760481f05ea40.
Are you adding it using the inline media feature or through the editor source view? I recommand you use the first way, see screenshot below , you will have access to fit to width or custom. If you want custom value, entering them in that dialog should work.

I noticed another issue, the PDF cannot be navigated, scrolling the Main Stage makes the whole app to scroll. I will look at that one for sure for next release and wait for your feedback on the other two.

Sorry for not being available to respond to this until n ow...
I am having better performance viewing the PDF as the main stage action so I think I will probably use that if it works OK with the resolution for our iframe (this will be embedded in a web page and I am using Ipad as the approx size of the frame).
For the other question, I am adding as you indicate in the screen shot-- I have done the 'fit to width' but it still seems to not fit the document itself but the PDF window that comes up. I will test it again because it looks good in your test app and that is how it should look in mine. It may be the way this window is getting compressed in the mobile view.
and finally, I did notice the PDF scroll issue. Luckily in my main stage action in the mobile view, it does work.
I'll be watching for the bug you reported and other improvements. thanks--

Wanted to let you know that I haven't been able to find a fix for any of those issues. I will be looking at it again for our next release in June.


model: relax restrictions on task names

Currently, task names can only contain spaces or alphanumeric characters. We should relax this restriction to allow users to store more varied task names.

Maybe at least allow punctuation marks?

@nightism What's the status of this issue anyway?


Deprecate enableStreaming

Description
As a result of https://github.com/Shopify/hydrogen/pull/1384, and as mentioned in this comment, the config option enableStreaming seems to be redundant with our (new) basic functionality. For the sake of not having 2 ways to do the same thing, this PR removes enableStreaming.
I've removed the docs but I'm not sure if we should cover the "custom bot" use case somewhere else. Currently, we mention about response.doNotStream() in routes.md, so maybe that's enough?
Additional context
Just to clarify, we were able to call doNotStream before but the response wouldn't be formatted for bots with nice title, meta, etc. That's the use-case that enableStreaming was (to my understanding) offering, a way to format the response for bots. This is now always the case when calling doNotStream.

Before submitting the PR, please make sure you do the following:

[x] Read the Contributing Guidelines
[x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
[x] Update docs in this repository according to your change
[x] Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

@frandiox I found one more link from the logger PR that could use a fix. Maybe we can make it here?
In preloaded_queries.md, we should update /api/hydrogen/utilities/log#logger-options to /custom-storefronts/hydrogen/framework/hydrogen-config#logger.


Project proposal : $uicideboy$ lyric analysis project.

I will be taking all core $B releases, tagging pseudonyms, key phrases, and common themes, which then will be arranged in a web format. Base statistics across releases, highlighting via verse, phrase, or title, and the ability to search for advanced statistics (how many times is ____ said across all releases etc.) This is an evolution of a multimedia project I did in Digit 100 where I took ten random songs and color coded the amount of pseudonym usage throughout the songs.

$B releases pre and post studio albums (ignoring singles, always on albums):
https://www.albumoftheyear.org/artist/11259-suicideboys/
Resource for finding lyrics :
https://genius.com/artists/Uicideboy
noted that any release has a lyrics page from google alone.


feat: default dockerfile cmd to serve

This changes the default behavior from help (no command) to serve when starting a container. Many servers such as postgresql, redis and more uses a similar pattern ("sane defaults"). Also, update documentation where relevant.
It is added as a new Dockerfile to prepare desting.
Note: this will not break existing behavior since passing a command when starting the container will override serve.

Hi @jbergstroem, i think you mentioned a good point. We can work on this. For further improvements we can also add custom flags in CMD to make it easier for you to update in workflow files. Also, what about creating a new Dockerfile.actions file to work on and test this?

Hi @jbergstroem, i think you mentioned a good point. We can work on this.

Cool! Added documentation.

For further improvements we can also add custom flags in CMD to make it easier for you to update in workflow files.

What do you mean by this? What should change?

Also, what about creating a new Dockerfile.actions file to work on and test this?

How would that look compared to the existing one? Can't we build and use that one in CI? Should this be e2e tests where we spin up postgres and so on?

Hi @jbergstroem, i think you mentioned a good point. We can work on this.

Cool! Added documentation.

For further improvements we can also add custom flags in CMD to make it easier for you to update in workflow files.

What do you mean by this? What should change?

Also, what about creating a new Dockerfile.actions file to work on and test this?

How would that look compared to the existing one? Can't we build and use that one in CI? Should this be e2e tests where we spin up postgres and so on?

Just a followup: we had a brief conversation over Discord and we'll start with a separate Dockerfile we can test with.

@ucatbas quick question - whats next here?

@jbergstroem thank you for the contribution!


Fix TypeError: Cannot read properties of undefined (reading 'length')

Hi! @elaichenkov  We have found the next error when integrating the plugin with cypress-cloud in run mode. Here we have a fix to read the original error from the spec.

I take this opportunity to ask you, should the plugin work in run mode, or only in open mode?

Hi @DavidFontalba
Thank you for your contribution!

I take this opportunity to ask you, should the plugin work in run mode, or only in open mode?

Only in open mode for now

@elaichenkov Until the merge and your last commits we have noticed that this is working too in screenshots for the run mode 😳 :partying_face:


Use most recent Bioconductor for Dockerfile

Fixes #7
When a new project is created, the generated Dockerfile should use the most recent Bioconductor version.  The Docker
API is used to get the version that is inserted into the Dockerfile.
Setup:
cd ~/bin/capture
git checkout main
git pull
git submodule update --init --recursive
git checkout latest-docker
cd project-template
git checkout latest-docker

Test:
Create a new project in your projects directory and check that the bin/docker/Dockerfile file has the latest version of Bioconductor.  The first line should look something like the following:
FROM bioconductor/bioconductor_docker:RELEASE_3_20-R-4.4.2

Test building a new docker using the commands in the SOP: Generating a new project with CAPTURE
Reset:
Run the following command to reset your environment back the released version of CAPTURE.
cap update

so we need cap installed locally as well as on the HPC for this, correct?

so we need cap installed locally as well as on the HPC for this, correct?

@adc0032 No, you will only clone the repo locally to build the docker?


WIP: GHC 9.2 support

hi @srid .
I don't think we're quite ready to merge this in main yet.
Apropos to which, i'd also like to request that you refrain from opening up GHC 9.2 upgrade related PRs (this and see). I'll make them where they're required and when they're ready.
If you'd like to raise an issue, please use the repository's issue tracker or open an RFC for comments, not a PR.
Thanks.
Closing.


test(dm): fix using pgrep to check argument pattern

Signed-off-by: lance6716<EMAIL_ADDRESS>
What problem does this PR solve?

Issue Number: close #6526, close #5731
What is changed and how it works?
we use kill_process dm-worker1 in test script, and finally it will become pgrep dm-worker1. This is not right because dm-worker1 is in arguments and pgrep only check process name.
Check List
Tests

Integration test
Manual test (add detailed scripts or steps below)

# lance @ lance6716-nuc10i7fnh in ~/go/src/github.com/pingcap/tiflow on git:master o [14:31:49]
$ pgrep -f dm-worker1
467807

# lance @ lance6716-nuc10i7fnh in ~/go/src/github.com/pingcap/tiflow on git:master x [14:32:09]
$ pgrep dm-worker1

Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
 `None`.

/cc @GMHDBJD @buchuitoudegou @D3Hunter

/check-issue-triage-complete

/run-verify

/run-dm-integration-test

/hold

/run-dm-integration-test

/unhold

/merge

/run-dm-integration-test
/run-dm-compatibility-test

pgrep with no flags only reads the first 15 characters of the executable's name.
so pkill dm-master.test.previous will fail.
The diff coverage is 58.0152%.

Flag
Coverage Δ

cdc
65.9320% <60.6289%> (-0.1810%)
:arrow_down:

engine
63.5261% <46.2809%> (-0.2156%)
:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.
@@               Coverage Diff                @@
##             master      #6572        +/-   ##
================================================
- Coverage   59.4214%   59.3559%   -0.0656%
================================================
  Files           763        777        +14
  Lines         87418      88185       +767
================================================
+ Hits          51945      52343       +398
- Misses        30894      31218       +324
- Partials       4579       4624        +45

/run-all-tests

We retry /run-dm-integration-test so many times. Do you file an issue tracking the unstable case?
@lance6716

We retry /run-dm-integration-test so many times. Do you file an issue tracking the unstable case?
@lance6716

We retry /run-dm-integration-test so many times. Do you file an issue tracking the unstable case? @lance6716

Yes, most of them are old issues, and I also recorded new failing cases.

/merge

/run-dm-integration-test

/merge

/run-dm-integration-test

/run-all-tests


Request: OAuth integration

Would it be possible to add OAuth authentication?  It would be useful to be able to authenticate against our organization's Google or Office365 accounts.

@jamesboehmer would you be willing to contribute that? :)
The authentication module should be more or less easy to extend implement and I could help out in laying any needed groundwork.
But implementing that by myself would not be in my top priorities and would likely not happen anytime soon.

@lmenezes Can you point me to the files of interest.  We might be interested in providing an implementation for use with Openshift Oauth workflow.


DocsArchive-<IP_ADDRESS>Auto archive content

Please don't merge this PR until target archive repo PR https://github.com/OPS-E2E-PPE/docs-archive-test-target_8cf98b9a-b14b-4217-ab77-0719ca023375/pull/1 is merged into live branch.
Auto archive content to https://github.com/OPS-E2E-PPE/docs-archive-test-target_8cf98b9a-b14b-4217-ab77-0719ca023375.git

Docs Build status updates of commit c11947e:
:x: Validation status: errors
Please follow instructions here which may help to resolve issue.

File
Status
Preview URL
Details

:x:Error

Details

[Error]

Cannot sync git repo to specified commit because branch Release_Archive_master_2020-02-10-07-05-20 has been deleted or has been force pushed: fatal: couldn't find remote ref refs/heads/Release_Archive_master_2020-02-10-07-05-20

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.


Improving prompt handling logic

Hey @yujonglee ,
Adding support for more advanced prompt handling logic through litellm config. It checks the number of tokens in the prompt and max tokens for each model. It selects the model with max tokens > prompt tokens.
If the prompt is larger than any available model, it'll automatically trim the prompt (from the middle + preserve any system message), and fit it to the largest model available.
Happy to add additional testing / docs if the initial PR looks good.
Open to any feedback/suggestions on this.
docs: https://docs.litellm.ai/docs/completion/config

@krrishdholakia  Thank you!


Use output log out/log/run_workflow

We are writing a small log in out/log/run_workflow.
Currently, the content of that file after running is just a list of subjects processed.
I would love to have a file like that with all the output from processing (preferably with .txt extension so that it is convenient to open with the browser from, i.e. circle's artifacts).

We should log an explicit INFO log of which workflow is being run (ds005 or ds054).


Add which::Path, which::CanonicalPath structs

This is something I've worked on in a potential downstream crate, but I think it's best suited for which-rs. I'm happy to consider naming suggestions.

Thanks. I need some time to review the new code.

I don’t think this is a good design, because it adds new types that don’t actually guarantee anything extra. You cannot depend on the contents of the file system not changing. They therefore signal intent only, but I don’t think that it’s a useful sort of intent to signal in types, especially when the naming is then confusing (Path being an owned type, unlike std::path::Path). I reckon this is a misfeature.

@chris-morgan Thanks for the feedback! I think this is debatable. Here are some thoughts.
Suppose which::which gave you a path: std::path::PathBuf. Since you can't depend on the contents of the file system, the file/directory referenced by path may be (re-)moved and, therefore, leave path invalid. How is this different from using which::Path::new? With the latter, you at least know that the path: which::Path came from which::which even if you don't know whether the target still exists by the time you use it. So it seems like there is something extra achieved by which::Path over std::path::PathBuf.
As to naming, I don't have a good answer there. I think which::Path should be immutable; otherwise, you lose the reason for the existence of the unique type. I don't think which::PathBuf is appropriate, because which::Path is immutable and should probably not have a Buf in its name.


Refactor: Unify Expr::ScalarFunction and Expr::ScalarUDF

Which issue does this PR close?

Part of https://github.com/apache/arrow-datafusion/issues/8157
Rationale for this change

See #8157 for the rationale:
Creation of Exprs for BuiltinScalarFunctions can be done statelessly
let expr1 = abs(lit(-1));
let expr2 = call_fn("abs", vec![lit(-1)]);

However, we can't do that for ScalarUDFs because they are registered within SessionState, it's only possible to do
let ctx = create_context()?;
ctx.register_udf(abs_impl);
let expr2 = ctx.call_fn("abs", vec![lit(-1)]);

For the ongoing plan to migrate functions from BuiltinScalarFunction to ScalarUDF based implementation (ref #8045 ), we would like the ScalarUDF based functions to continue to support the original Expr construction API, so the Expr struct should have a variant for unresolved name, and then we can add a AnalyzerRule to resolve the function.
This PR only include refactor preparation for UDF Expr support.
What changes are included in this PR?

Change Expr::ScalarFunction

// before
pub struct ScalarFunction {
    /// The function
    pub fun: built_in_function::BuiltinScalarFunction,
    /// List of expressions to feed to the functions as arguments
    pub args: Vec<Expr>,
}
// This PR
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ScalarFunctionDefinition {
    /// Resolved to a `BuiltinScalarFunction`
    /// There is plan to migrate `BuiltinScalarFunction` to UDF-based implementation (issue#8045)
    /// This variant is planned to be removed in long term
    BuiltIn(built_in_function::BuiltinScalarFunction),
    /// Resolved to a user defined function
    UDF(Arc<crate::ScalarUDF>),
    /// A scalar function constructed with name, could be resolved with registered functions during
    /// analyzing.
    Name(Arc<str>),
}

/// ScalarFunction expression invokes a built-in scalar function
#[derive(Clone, PartialEq, Eq, Hash, Debug)]
pub struct ScalarFunction {
    /// The function
    pub func_def: ScalarFunctionDefinition,
    /// List of expressions to feed to the functions as arguments
    pub args: Vec<Expr>,
}

Remove Expr::ScalarUDF

This way we can support unresolved function name variant.
Also, merge the execution path for BuiltinScalarFunction/ScalarUDF, to make future work towards #8045 a bit easier
Are these changes tested?
Covered by existing tests.

Are there any user-facing changes?
Yes, see comments for API changes

@alamb  Thank you! Review comments are addressed